Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables

core/modules/views/src/Plugin/views/area/Result.php

render() function has lot of unused code.

CommentFileSizeAuthor
#23 rewrite_the_result-2598436-23.patch4.38 KBkostyashupenko
#9 2598436-7.patch4.38 KBdawehner
#5 2598436-5.patch1.15 KBdawehner
#2 2598436-1.patch1.42 KBkrknth

Comments

krknth created an issue. See original summary.

krknth’s picture

krknth commented
Status: Needs work » Needs review
StatusFileSize
new 2598436-1.patch1.42 KB

Added patch

krknth’s picture

krknth commented
Assigned: krknth » Unassigned
Issue tags: +Less code
krknth’s picture

krknth commented
Issue tags: +Novice
dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2598436-5.patch1.15 KB

I'm sorry but this breaks code ... its not easy to see but this "${$item}" is its own special magic.

Here is a fix that we never run into the problem agian

krknth’s picture

krknth commented

oops! :)

The last submitted patch, 2: 2598436-1.patch, failed testing.

krknth’s picture

krknth commented

Hiding my patch as its wrong & to stop system bot taking up these.

sorry for inconvenience

dawehner’s picture

dawehner
Primary language German
commented
Title: Remove unused variables/code from core/modules/views/src/Plugin/views/area/Result.php » Rewrite the Result class to no longer accidentally be removed by people.
StatusFileSize
new 2598436-7.patch4.38 KB

No worries, its a good sign that we actually should better fix this shitty piece of code in the right way.

Also did some small test cleanups.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Looks good. Really glad that #2 failed :)

The last submitted patch, 5: 2598436-5.patch, failed testing.

The last submitted patch, 2: 2598436-1.patch, failed testing.

The last submitted patch, 5: 2598436-5.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2598436-7.patch, failed testing.

krknth’s picture

krknth commented
Status: Needs work » Reviewed & tested by the community

Added test & passing
Moving 'Needs work' to RTBC as it moved by BOT

krknth’s picture

krknth commented

Hiding patch that no longer required

The last submitted patch, 2: 2598436-1.patch, failed testing.

The last submitted patch, 5: 2598436-5.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Yeah this issue doesn't have to land before 8.0.0 but can certainly land afterwards

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2598436-7.patch, failed testing.

The last submitted patch, 9: 2598436-7.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs reroll
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new rewrite_the_result-2598436-23.patch4.38 KB

Re-rolled patch from comment #9 with auto merge

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

per #15

  • catch committed 1096471 on 
    Issue #2598436 by dawehner, krknth, kostyashupenko: Rewrite the Result...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

This is pure refactoring, so not committing to 8.0.x - re-open if you disagree.

dawehner’s picture

dawehner
Primary language German
commented

Yeah I don't care about 8.0.x
This code would also not be removed accidentally in 8.0.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.