Node moved into a different microsite appears in original microsite menu

Thanks for reporting, are you able to roll a patch and expand the tests for the bug?

Hey @larowlan,
I'm hoping to post a patch with test coverage soon. Depending on bandwidth though, we might start by posting a patch with just the proposed fix so we can use it on a project right away. I'll take care of the test at some point though.

Cheers!

@larowlan, here's a patch with enhanced tests for testing the EntityHooks class. All four of the new assertions fail. The final one two are the ones most related to the issue as I described it, but it looks like there are other problems in EntityHooks that are worth addressing.

Would you be able to review the tests and confirm that the new assertions make sense? Are those reasonable expectations?

I didn't call $this->triggerMenuRebuild() anywhere in the tests because that's something that is supposed to happen automatically based on EntityHooks.

I might be misunderstand what should happen to grandchildren if I delete the parent. I expected the grandchildren to leave the microsite. But even if I trigger a menu rebuild before each page view, orphaned children are staying in the microsite menu. Is that expected?

The last submitted patch, 5: microsite-entityhooks-3359425-5.patch, failed testing. View results →

Back to NR to get @larowlan's eyes on the tests to confirm the new assertions are reasonable.

So yes, if you delete an ancestor, its dependents get adopted by the ancestor's parent

E.g if a parent link is removed, the grandparents adopt it 👨‍🍼

@larowlan, Thank you kindly for that tidbit. That means a couple of the new assertions are not correct. I'll rewrite the new coverage to account for the adoption.

Adding a related issue: #3212256: Microsite menu links not removed when deleteing nodes →

Here's an updated tests with corrected assertions (per #8) and yet another new assertion where a node is removed from any microsite at the end. This fail test passes if we trigger a menu rebuild before doing each drupalGet. But we shouldn't have to do that if EntityHooks is working optimally. And they each fail without doing the menu rebuild. We'll only get to see the first one fail obviously.

This new test coverage is clearly beyond the scope of the issue summary, but I hate to just add coverage for my small case where there appear to be bugs with adding nodes and deleting nodes as well as moving nodes. It's all in EntityHooks and it might all be interconnected.

The last submitted patch, 11: microsite-entityhooks-3359425-11-FAIL.patch, failed testing. View results →

1. +++ b/modules/entity_hierarchy_microsite/tests/src/Functional/MicrositeMenuBlockTest.php
   @@ -65,6 +65,61 @@ class MicrositeMenuBlockTest extends MicrositeFunctionalTestBase {
   +    // Test EntityHooks by inserting, updating, and deleteing content.
   

   nit, I think its deleting, not deleteing

2. +++ b/modules/entity_hierarchy_microsite/tests/src/Functional/MicrositeMenuBlockTest.php
   @@ -65,6 +65,61 @@ class MicrositeMenuBlockTest extends MicrositeFunctionalTestBase {
   +    // The menu should contain links to all 11 children/grandchildren.
   

   Do we need a triggerRebuild after creating the children via the api? I think so, as we're not submitting a form so there's no kernel terminate

3. +++ b/modules/entity_hierarchy_microsite/tests/src/Functional/MicrositeMenuBlockTest.php
   @@ -65,6 +65,61 @@ class MicrositeMenuBlockTest extends MicrositeFunctionalTestBase {
   +    $children['Child 2']->delete();
   

   after this too I think

4. +++ b/modules/entity_hierarchy_microsite/tests/src/Functional/MicrositeMenuBlockTest.php
   @@ -65,6 +65,61 @@ class MicrositeMenuBlockTest extends MicrositeFunctionalTestBase {
   +    $children['Child 1']->set(static::FIELD_NAME, ['target_id' => $second_root->id(), 'weight' => 0])->save();
   

   And after this too

5. +++ b/modules/entity_hierarchy_microsite/tests/src/Functional/MicrositeMenuBlockTest.php
   @@ -65,6 +65,61 @@ class MicrositeMenuBlockTest extends MicrositeFunctionalTestBase {
   +    $children['Child (child of 1) 1']->set(static::FIELD_NAME, NULL)->save();
   

   and this

This new test coverage is clearly beyond the scope of the issue summary, but I hate to just add coverage for my small case where there appear to be bugs with adding nodes and deleting nodes as well as moving nodes. It's all in EntityHooks and it might all be interconnected.

We have a lot of existing coverage in \Drupal\Tests\entity_hierarchy_microsite\Kernel\MicrositeMenuItemsTest, I'd prefer to add to that rather than do it in a functional test.

Any reason we can add to what's there?

Hi, @larowlan,
Thanks for the thorough comments.

Do we need a triggerRebuild after creating the children via the api? I think so, as we're not submitting a form so there's no kernel terminate

Boy, this is getting to the edge of my abilities here...it took about 45 minutes but I finally understand what you're saying here. I had not thought deeply about what triggerMenuRebuild was doing. To me it looked like using this method was hiding bugs in EntityHooks. That's why I was intentionally avoiding its usage. Instead, this method is just accounting for the fact that MenuRebuildProcessor::destruct only happens at the end of a request. Holy moly.

We have a lot of existing coverage in \Drupal\Tests\entity_hierarchy_microsite\Kernel\MicrositeMenuItemsTest, I'd prefer to add to that rather than do it in a functional test.

I was specifically avoiding the Kernel test because my (mis)understanding was that the hooks that call the EntityHooks methods wouldn't run in a Kernel test. (Why did I think that? Are there some hooks that don't fire in a Kernel test?) I did some Xdebugging though and proved I was mistaken. I agree that adding to the Kernel test would be better.

I need to take a step back. As I mentioned in #11 "This fail test passes if we trigger a menu rebuild before doing each drupalGet". Does that mean there's not really a bug? Or does it mean my "fail" test was not doing what was needed to expose the bug. I need to do the manual testing again.

I did some careful manual testing and could not reproduce this issue. I wonder if I was seeing something that was not general, but was instead related to 🐛 EntityHooks ignore node that can only be a leaf in a microsite Active .

I'm going to close this issue.

However, I think I've learned a lot here and will be able to address 🐛 EntityHooks ignore node that can only be a leaf in a microsite Active at some point. And potentially #3212256: Microsite menu links not removed when deleteing nodes → which appears to be real as well.

Thanks for all the help, @larowlan!

Thanks @danflanagan8, appreciate the attention to detail