Problem/Motivation

When adding variants to a page, we add fill the Page::$variants array. If the variants haven't already been initialised via Page::getVariants() then Page::getVariant(), Page::getVariants() and Page::removeVariant() will never get any other variants that exist on the page and therefore return incomplete lists of variants for Page::getVariants() or throw UnexpectedValueException for the other methods.

In addition to this, even if we have already initialised Page::$variants, the addition could leave them out of order which wont get corrected until a fresh copy of the page is retrieved and the variants are loaded fresh from the database.

When removing a variant, the variant is deleted, but never removed from Page::$variants which could also cause similar problems.

Proposed resolution

  • Abstract sorting variants into a separate method
  • Always initiate the variants before adding
  • Always sort the variants after adding
  • Unset the removed variant from Page::$variants
CommentFileSizeAuthor
#11 fix_add_remove-2692397-8.patch5 KBandrewbelcher
#11 fix_add_remove-2692397-8-test_only.patch3.58 KBandrewbelcher
#6 fix_add_remove_with_sort_change-2692397.patch7.19 KBandrewbelcher
#6 fix_add_remove_with_sort_change-2692397-test_only.patch5.43 KBandrewbelcher
#3 fix_add_remove-2692397.patch5.19 KBandrewbelcher
#3 fix_add_remove-2692397-test_only.patch3.58 KBandrewbelcher

Comments

andrewbelcher created an issue. See original summary.

andrewbelcher’s picture

andrewbelcher commented
Title: https://www.drupal.org/node/add/project-issue/page_manager » Always initiate/sort variants on adding

Fixing title...

andrewbelcher’s picture

andrewbelcher commented
Title: Always initiate/sort variants on adding » Adding/removing variants leaves Page::$variants out of date
Issue summary: View changes
StatusFileSize
new fix_add_remove-2692397-test_only.patch3.58 KB
new fix_add_remove-2692397.patch5.19 KB

There is also a similar issue with removing (see #2692347: removeVariant() should remove variant from page and not just delete the variant. which I've merged into this). Updated issue summary accordingly.

Here are a couple patches, one test only which demonstrates the problem and one with a fix that abstracts the sorting into Page::sortVariants() so it can be called from Page::addVariant.

andrewbelcher’s picture

andrewbelcher commented
Status: Active » Needs review

The last submitted patch, 3: fix_add_remove-2692397-test_only.patch, failed testing.

andrewbelcher’s picture

andrewbelcher commented
StatusFileSize
new fix_add_remove_with_sort_change-2692397-test_only.patch5.43 KB
new fix_add_remove_with_sort_change-2692397.patch7.19 KB

There is also a question of whether we should switch over to using PageVariant::sort() (which is actually \Drupal\Core\Config\Entity\ConfigEntityBase::sort) as our sorting callback, which means we can drop Page::variantSortHelper.

This adds in label as a secondary sorting mechanism. It's possible this may not actually be desirable. If it isn't, I would still suggest the change I've made, but overriding PageVariant::sort() to not check the label as that seems a better design pattern.

I've uploaded patches that switch to using PageVariant::sort() including testing the addition sort based on label.

The last submitted patch, 6: fix_add_remove_with_sort_change-2692397-test_only.patch, failed testing.

juampynr’s picture

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

I tested this patch in conjunction with #2550879: Use CTools Wizard API to add/edit Pages (and move plugin UI using PluginWizardInterface) and it works fine, plus all tests pass.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
  1. +++ b/src/Entity/Page.php
@@ -343,23 +350,19 @@ class Page extends ConfigEntityBase implements PageInterface {
-  public function variantSortHelper($a, $b) {
...
+      @uasort($this->variants, [PageVariant::class, 'sort']);

    PageVariant::sort and variantSortHelper have different logic. If you want to remove this helper, then please move this logic (getWeight() calls only) to override PageVariant::sort() 

  2. +++ b/tests/src/Unit/PageTest.php
@@ -49,15 +49,71 @@ class PageTest extends UnitTestCase {
-    $variant2->getWeight()->willReturn(-10);
...
+    $variant2->weight = -10;

    I finally understand this (still wrong) change. It's because \Drupal\Core\Config\Entity\ConfigEntityBase::sort() uses ->weight directly, we want to use getWeight().

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests
andrewbelcher’s picture

andrewbelcher commented
Status: Needs work » Needs review
StatusFileSize
new fix_add_remove-2692397-8-test_only.patch3.58 KB
new fix_add_remove-2692397-8.patch5 KB

Ok, based on an IRC conversation, sounds like the consensus is to not deal with changing the way we actually do the sort here, but rather in a follow up which I'll post shortly.

In the light of that, here is the patch from #3 with a minor fix and I'll hide everything else.

The last submitted patch, 11: fix_add_remove-2692397-8-test_only.patch, failed testing.

andrewbelcher’s picture

andrewbelcher commented

Have opened #2693165: Move variant sort callback into PageVariant as a follow up for moving our sort helper.

andrewbelcher’s picture

andrewbelcher commented
Related issues: +#2693165: Move variant sort callback into PageVariant
juampynr’s picture

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

Good job @andrewbelcher!

eclipsegc’s picture

eclipsegc commented
Status: Reviewed & tested by the community » Fixed

I concur. Fixed

Status: Fixed » Closed (fixed)

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