Problem/Motivation

#3284026: Menu Link Discovery takes forever on huge websites switched from a granular approach to updating menus to a rebuild approach, however this can cause unnecessary menu rebuilds when the parent hasn't changed
There is code in the EntityHooks class to prevent rebuilding on every update if the parent and weight didn't change, but the code doesn't work, so each update of a node in a microsite causes a menu regeneration

Steps to reproduce

Resave an item in a microsite, notice the menu rebuilds.

Proposed resolution

Fix the code so we don't rebuild the menu if the parent didn't change.
Add tests

Remaining tasks

Code, tests

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 3322336-3-gah-php-7.patch20.79 KBlarowlan
#8 3322336-3-interdiff.txt713 byteslarowlan
#6 3322336-2.patch20.79 KBlarowlan
#6 3322336-2-interdiff.txt18.99 KBlarowlan
#3 3322336-pass.patch8.69 KBlarowlan
#3 3322336-fail.patch5.42 KBlarowlan

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
StatusFileSize
new 3322336-fail.patch5.42 KB
new 3322336-pass.patch8.69 KB

The last submitted patch, 3: 3322336-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 3: 3322336-pass.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
StatusFileSize
new 3322336-2-interdiff.txt18.99 KB
new 3322336-2.patch20.79 KB

Now with 100% less shutdown function rubbish 🤮

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
  1. +++ b/modules/entity_hierarchy_microsite/src/Plugin/Block/MicrositeMenu.php
@@ -92,7 +92,6 @@ class MicrositeMenu extends SystemMenuBlock implements ContainerFactoryPluginInt
-        $menu_trail_ids = array_reverse(array_values($parameters->activeTrail));

    this was unused 

  2. +++ b/modules/entity_hierarchy_microsite/tests/modules/entity_hierarchy_microsite_test/entity_hierarchy_microsite_test.module
@@ -35,3 +36,10 @@ function entity_hierarchy_microsite_test_entity_bundle_info_alter(&$bundles) {
+  \Drupal::state()->set(MicrositeMenuItemsTest::STATE_KEY, \Drupal::state()->get(MicrositeMenuItemsTest::STATE_KEY, 0) + 1);

    track number of rebuilds

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
StatusFileSize
new 3322336-3-interdiff.txt713 bytes
new 3322336-3-gah-php-7.patch20.79 KB

(╯°□°)╯︵ ┻━┻ damn php 7 support

mstrelan’s picture

mstrelan commented

Looks pretty good to me although I'm not overly familiar with the domain. The cs changes made it harder to see what's new, but that's mostly what I have to comment on.

  1. +++ b/modules/entity_hierarchy_microsite/src/EntityHooks.php
@@ -59,6 +59,11 @@ class EntityHooks implements ContainerInjectionInterface {
+  /**
+   * @var \Drupal\entity_hierarchy_microsite\MenuRebuildProcessor
+   */

    I think we don't need @var tags on typed properties. See #3123282: Do not require @var tag if a property has typehint. 

  2. +++ b/modules/entity_hierarchy_microsite/src/EntityHooks.php
@@ -72,13 +77,23 @@ class EntityHooks implements ContainerInjectionInterface {
+    MenuRebuildProcessor $menuRebuildProcessor

    Do you need to provide a default value and deprecate not passing it? 

  3. +++ b/modules/entity_hierarchy_microsite/src/MenuRebuildProcessor.php
@@ -0,0 +1,59 @@
+  /**
+   * TRUE if needs rebuild.
+   *
+   * @var bool
+   */
+  protected $needsRebuild = FALSE;
+
+  /**
+   * Menu link manager.
+   *
+   * @var \Drupal\Core\Menu\MenuLinkManagerInterface
+   */
+  protected $menuLinkManager;

    Do you want to declare types here? 

  4. +++ b/modules/entity_hierarchy_microsite/tests/src/Traits/MenuRebuildTrait.php
@@ -0,0 +1,19 @@
+   * Triggers menu rebuilding if its needed.

    Nit: it's

nterbogt’s picture

nterbogt commented

I've completed some end to end testing on the patch and it's working as expected, for our use cases.

pghaemim’s picture

pghaemim commented

looks good to me. Thanks

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Fixed

#9.1 we support php7,3 still (for now)
#9.2, no this is internal like all hook implementations (in my book)
#9.3 same re 7.3
#9.4 will fix on commit

Added #3324454: Remove php 7 support for #9 items 1 and 2

Cutting 3.3.3 with this in it

  • larowlan committed c149280 on 3.x 
    Issue #3322336 by larowlan, mstrelan, nterbogt, pghaemim: Don't update...

Status: Fixed » Closed (fixed)

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