Reverting a Layout doesn't work if you have adding new block to the layout before you click "Revert to defaults". though the message still says you reverted. Here is test addition for that. It will fail now.

CommentFileSizeAuthor
#16 2970801-revert-15.patch3.08 KBtim.plunkett
#16 2970801-revert-15-interdiff.txt1.66 KBtim.plunkett
#10 interdiff-2970801-5-10.txt1.42 KBvictor-shelepen
#10 2970801-10.patch5.81 KBvictor-shelepen
#5 2970801-5.patch4.65 KBtedbow
#5 interdiff-4-5.txt4.11 KBtedbow
#4 2970801-4.patch2.15 KBtedbow
#4 interdiff-2-4.txt831 bytestedbow
#2 layout_revert_test.patch1.34 KBtedbow
layout_revert_test.patch1.34 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
layout_revert_test.patch1.34 KB

Status: Needs review » Needs work

The last submitted patch, 2: layout_revert_test.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-2-4.txt831 bytes
2970801-4.patch2.15 KB

Not sure why this works but posting to see if anyone else knows.

got this idea from @johndevman

Who was testing and found the test passes if

  1. It works if the code in submitForm() is moved to buildForm()
  2. It also works, if you reload the storage from the storage manager in the submitForm()

1) didn't seem like good idea so tried 2)

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-4-5.txt4.11 KB
2970801-5.patch4.65 KB

Ok, this fix makes sense because \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::getEntity it going to get the entity with the sections before the latest block is added.

I cleaned up the fix to use inject the section storage manager. I am keeping loading the section storage manager in submitForm() because it should be loaded at the last possible time before reverting.

Also added commented in the test to explain why we add a block.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
CreditAttribution: sjerdo as a volunteer and at Synetic commented
Status: Needs review » Reviewed & tested by the community

Works like a charm. Code seems good.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Reviewed & tested by the community » Needs review

@sjerdo thanks for testing this out

@tim.plunkett wanted to take a look at this because we might need to change some routing level items not send the $section_storage to ::buildForm()

So setting back to needs review.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Needs work

NW for #7

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

victor-shelepen’s picture

victor-shelepen CreditAttribution: victor-shelepen as a volunteer commented
Status: Needs work » Needs review
FileSize
2970801-10.patch5.81 KB
interdiff-2970801-5-10.txt1.42 KB

I checked the 5th patch. It worked well. I did some modifications. The $section_storage has been moved to ::buildForm( ). So we do need pass $section_storage as a parameter on Route API level that is mentioned in [#2970801#7]

victor-shelepen’s picture

victor-shelepen CreditAttribution: victor-shelepen as a volunteer commented

The current node is getting from the route. It can also be gotten from the context handler. It is not clear how to get a section storage type and an entity type if it is needed for another cases. Now they are variables defined strictly.

victor-shelepen’s picture

victor-shelepen CreditAttribution: victor-shelepen as a volunteer commented
Issue tags: +sprint

The tag has been added to make visible the issues on the sprint board.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Needs work
Issue tags: +MWDS2018, +Needs tests

When running the test from #10 by itself (without the fix), it does not fail. This means that the test does not correctly expose the bug.
Working on this with @sugaroverflow at MWDS.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Aha! That was because a workaround for this issue was added yesterday in the inline blocks commit :)
#2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder

tim.plunkett credited sugaroverflow.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2970801-revert-15-interdiff.txt1.66 KB
2970801-revert-15.patch3.08 KB

So the tests were fine after all.
Removing the workaround added in RevertOverridesForm makes the test fail, as expected.

Earlier our theory was that this was a bug with tempstore. And reloading the entity would "fix" it.
However the bug seems to be in our LayoutSectionItem implementation.
We consult the entity for the field, and then pass the field around. It itself can retrieve the entity.
Except that the entity it returns is not identical to the entity we are manipulating!

LayoutSectionItem already had a partial workaround for this:

  public function __wakeup() {
    // Ensure the entity is updated with the latest value.
    $this->getEntity()->set($this->getName(), $this->getValue());
  }

__wakeup() is a magic method that runs when the object is unserialized (like from tempstore!)
The field losing track of the correct entity values happens much later than unserialized.
The solution we came up with was to move the resyncing from __wakeup directly to its internal getEntity() method.

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs review » Reviewed & tested by the community

Test coverage, simple solution and manual testing works as expected.

The last submitted patch, 5: 2970801-5.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed e3c750b08f to 8.7.x and 14e7fb44a0 to 8.6.x. Thanks!

  • alexpott committed e3c750b on 8.7.x 
    Issue #2970801 by tedbow, tim.plunkett, likin, sugaroverflow: If you add...

  • alexpott committed 14e7fb4 on 8.6.x 
    Issue #2970801 by tedbow, tim.plunkett, likin, sugaroverflow: If you add...

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: +Blocks-Layouts

Retroactively tagging

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Retroactively tagging