/**
  * @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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

tedbow’s picture

Component: quickedit.module » outside_in.module
Issue tags: -Outside-in

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rajeevk’s picture

Assigned: Unassigned » rajeevk
Status: Active » Needs review
FileSize
1.21 KB

Attaching patch as per suggestion to rename protected property $entity to $menu.

naveenvalecha’s picture

Assigned: rajeevk » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks, 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

tedbow’s picture

Version: 8.3.x-dev » 8.4.x-dev

@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

Dinesh18’s picture

I have reviewed the patch and verified that $entity is re-named to $menu.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
@@ -35,7 +35,7 @@ class SystemMenuOffCanvasForm extends PluginFormBase implements ContainerInjecti
   /**
    * @var \Drupal\system\MenuInterface
    */
-  protected $entity;
+  protected $menu;

This 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
2.46 KB

Re #8 @alexpott yep you are right.

Removed all uses of $this->entity with $this->menu

Dinesh18’s picture

Status: Needs review » Reviewed & tested by the community

As per the comment #8, I have reviewed the patch #9 and $this->entity is replaced with $this->menu.
Looks like RTBC.

  • Gábor Hojtsy committed 08f5c1f on 8.3.x
    Issue #2785135 by tedbow, RajeevK, effulgentsia, alexpott: Rename...

  • Gábor Hojtsy committed 5c84cb1 on 8.4.x
    Issue #2785135 by tedbow, RajeevK, effulgentsia, alexpott: Rename...
Gábor Hojtsy’s picture

Title: Rename protected property SystemMenuOffCanvasForm::$entity to $menu? » Rename protected property SystemMenuOffCanvasForm::$entity to $menu
Status: Reviewed & tested by the community » Fixed

Thanks, looks good now. I agree this makes it easier to understand.

tedbow’s picture

@Dinesh18 thanks or review the issue!

@Gábor Hojtsy thanks for committing!

Status: Fixed » Closed (fixed)

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

tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)