Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
/**
* @var \Drupal\system\MenuInterface
*/
protected $entity;
If it's a MenuInterface object, would it be clearer to name it $menu? When reviewing the code, I got tripped up on it being named $entity, because I kept thinking it was referring to the block entity, rather than to the menu entity. Block plugins shouldn't refer to block entities, so one could argue I was being tripped up for no good reason, but it happened anyway.
Comment | File | Size | Author |
---|---|---|---|
#9 | interdiff-2785135-4-9.txt | 2.46 KB | tedbow |
#9 | 2785135-9.patch | 3.11 KB | tedbow |
#4 | 2785135-4.patch | 1.21 KB | rajeevk |
Comments
Comment #2
tedbowComment #4
rajeevkAttaching patch as per suggestion to rename protected property $entity to $menu.
Comment #5
naveenvalechaThanks, Rajeev! Change Looks good. Marking it RTBC
This is the change in the Form class, As they are not the part of APIs as per BC but I'm not sure will it target 8.3
//Naveen
Comment #6
tedbow@RajeevK thanks for the patch!
@naveenvalecha yes I think this will go against 8.4.x and then get back ported because it is experimental module.
The test already rand again 8.4.x so good to stay at RTBC
Comment #7
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedI have reviewed the patch and verified that $entity is re-named to $menu.
Comment #8
alexpottThis change doesn't look right.
$this->menu
is never set and$this->entity
is the menu entity and is used plenty. The rest patch looks fine.Comment #9
tedbowRe #8 @alexpott yep you are right.
Removed all uses of
$this->entity
with$this->menu
Comment #10
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedAs per the comment #8, I have reviewed the patch #9 and $this->entity is replaced with $this->menu.
Looks like RTBC.
Comment #13
Gábor HojtsyThanks, looks good now. I agree this makes it easier to understand.
Comment #14
tedbow@Dinesh18 thanks or review the issue!
@Gábor Hojtsy thanks for committing!
Comment #16
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)