Problem/Motivation

\Drupal\layout_builder\SectionListInterface provides methods for manipulating a list of sections.
\Drupal\layout_builder\SectionStorageInterface extends that interface to provide methods to load and persist that list.

\Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase exists to provide a base class for implementations of SectionStorageInterface

\Drupal\layout_builder\SectionStorage\SectionStorageTrait exists to provide a trait for implementations of SectionListInterface

That name mismatch is rather confusing.

Proposed resolution

Move the code to a new trait named SectionListTrait and empty out and deprecate the old trait, importing/using the new one for BC.

Remaining tasks

Change record should be updated.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#50 interdiff_3035174_48-50.txt2.38 KBankithashetty
#50 3035174-50.patch29.57 KBankithashetty
#48 3035174-renametrait-48.patch29.64 KBtim.plunkett
#45 3035174-renametrait-45.patch29.73 KBtim.plunkett
#44 3035174-renametrait-44-interdiff.txt3.41 KBtim.plunkett
#44 3035174-renametrait-44.patch29.83 KBtim.plunkett
#43 3035174-43.patch32.14 KBmrinalini9
#36 3035174-36.patch30.21 KBandypost
#36 interdiff.txt1.29 KBandypost
#31 3035174-renametrait-29.patch31.04 KBtim.plunkett
#31 3035174-renametrait-29-interdiff.txt6.19 KBtim.plunkett
#26 3035174-26.patch32.59 KBjwwj
#14 3035174-14.patch31.77 KBandypost
#14 interdiff.txt1.46 KBandypost
#12 interdiff.txt4.77 KBandypost
#12 3035174-12.patch30.66 KBandypost
#10 3035174-renametrait-10.patch28.92 KBtim.plunkett
#7 3035174-renametrait-7.patch28.91 KBtim.plunkett
#6 3035174-renametrait-6.patch29.36 KBtim.plunkett
#3 3035174-renametrait-3.patch29.35 KBtim.plunkett
#2 3035174-renametrait-2.patch23.63 KBtim.plunkett

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new 3035174-renametrait-2.patch23.63 KB

Needs a CR and to have the deprecated messages link to it.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3035174-renametrait-3.patch29.35 KB

Reroll

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Needs change record

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3035174-renametrait-6.patch29.36 KB

Reroll

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +sprint
StatusFileSize
new 3035174-renametrait-7.patch28.91 KB

Reroll.
Still needs a CR, and the patch to be updated accordingly.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Issue tags: +DrupalCampBelarus2019
petu’s picture

petu commented

We are on DrupalCampBelarus2019 reviewing this issue with @kachinskiy.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3035174-renametrait-10.patch28.92 KB

Patch is stale but git was able to rebase this. No changes

Status: Needs review » Needs work

The last submitted patch, 10: 3035174-renametrait-10.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Issue tags: +Needs change notice
StatusFileSize
new 3035174-12.patch30.66 KB
new interdiff.txt4.77 KB

Fix test and clean-up for 8.8

Status: Needs review » Needs work

The last submitted patch, 12: 3035174-12.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Issue tags: -Needs change notice
StatusFileSize
new interdiff.txt1.46 KB
new 3035174-14.patch31.77 KB

Fix last failure

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented
Issue tags: +DCScot19
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Issue tags: +DC19PL

sandboxpl and shumer are sprinting on this issue at Drupal Camp Poland 2019 Contribution time.

sandboxpl’s picture

sandboxpl
Primary language Polish
Location Poland 🇵🇱
commented
Issue summary: View changes

Things we did:
- Fresh installation of Drupal 8.8
- Applied a patch #14
- Installed the layout builder module and enabled it for Article content type
- Adding and removing custom blocks, adding layout to the content, changing order of sections/blocks, adding/removing blocks to existing layout
- Reverting layout back to default version

For us the patch is working and we didn't find any issues during testing.
What should be done for the change record, where and how should it be edited?

martin107’s picture

martin107 commented

What should be done for the change record, where and how should it be edited?

Here is a guide to writing change records

https://www.drupal.org/contributor-tasks/draft-change-record

My advice -- keep it to the smallest number of sentences that full describe the change.
Many exiting change records read like a small set of bullets points.

I hope this helps ...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work
Issue tags: -DrupalCampBelarus2019, -DCScot19, -DC19PL

NW for the change record

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue tags: +Novice, +Amsterdam2019

Adding tags.

Please see the comments above about writing a change record.

jwwj’s picture

jwwj commented
Assigned: Unassigned » jwwj

I'll take a stab at writing the change record, mentored by mmbk

jwwj’s picture

jwwj commented
Status: Needs work » Active
jwwj’s picture

jwwj commented

First draft of change record written, but patch failed to apply on 8.8.x, 8.9.x branches. Will try creating a new patch.

mmbk’s picture

mmbk
Primary language German
Location Meißen
commented
Issue tags: +Needs reroll
jwwj’s picture

jwwj commented
StatusFileSize
new 3035174-26.patch32.59 KB

Rerolled patch from #14, should now apply to 8.9.x

martin107’s picture

martin107 commented
Issue tags: -Needs change record

Change record looks great

martin107’s picture

martin107 commented

Change record looks great

mmbk’s picture

mmbk
Primary language German
Location Meißen
commented
Issue tags: -Needs reroll

Patch applies to 8.9.x, tests succeded: Thanks you for your work, @jwwj

mmbk’s picture

mmbk
Primary language German
Location Meißen
commented
Status: Active » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3035174-renametrait-29-interdiff.txt6.19 KB
new 3035174-renametrait-29.patch31.04 KB 
+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -355,7 +355,7 @@ protected function getContextsForEntity(FieldableEntityInterface $entity) {
-    @trigger_error('\Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface::findByContext() should be used instead. See https://www.drupal.org/node/3022574.', E_USER_DEPRECATED);
+    @trigger_error('\Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface::findByContext() should be used instead. See https://www.drupal.org/node/3022574.', E_USER_DEPRECATED);

This change of the version is incorrect. It was already deprecated in 8.7 and that does not change with this patch.

This is also missing the test coverage from previous patches.

Status: Needs review » Needs work

The last submitted patch, 31: 3035174-renametrait-29.patch, failed testing. View results

jwwj’s picture

jwwj commented
Assigned: jwwj » Unassigned

stupid question, but where do I see the test coverage report in that case? Anyway, I'll change to unassigned in case somebody else wants to take a look at fixing the failing test before I can find time for it again.

mmbk’s picture

mmbk
Primary language German
Location Meißen
commented

The test result is here and is a direct result of changing the deprecation version, as the test expects the Version 8.8.0 to be deprecated. https://www.drupal.org/pift-ci-job/1455962

The first deprecation annotation I found was refering to Version 8.8.0, so I guess patch #26 is correct.

Edit: Meanwhile I dug into the code and found that #14 fixed the failed test of #12 at the wrong location. I'll provide the patch for this.

mmbk’s picture

mmbk
Primary language German
Location Meißen
commented
Status: Needs work » Active
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new interdiff.txt1.29 KB
new 3035174-36.patch30.21 KB

Fix failed test caused by #31

mmbk’s picture

mmbk
Primary language German
Location Meißen
commented

Hmm, just wanted to upload the new patch, and I saw that the tests are already running :-(

Nevertheless. I tried to figure out what was wrong with the test-coverage

This is also missing the test coverage from previous patches.

What is wrong with the tests, that were included in #26 and removed in #31? The tests passed during the testrun of #29 and they seem to be resonable.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -sprint, -Novice, -Amsterdam2019

Still needs a review.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php
@@ -2,184 +2,20 @@
+ * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use

Looks that no go for 8.9 now and should be moved to 9.1

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll for 9.1/10.0

mrinalini9’s picture

mrinalini9 commented
Assigned: Unassigned » mrinalini9
mrinalini9’s picture

mrinalini9 commented
Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3035174-43.patch32.14 KB

Rerolled patch for 9.1.x, please review.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -Needs reroll
StatusFileSize
new 3035174-renametrait-44.patch29.83 KB
new 3035174-renametrait-44-interdiff.txt3.41 KB

#43 is missing the necessary changes to SectionStorageTestBase and doesn't deprecate it at all.

Rerolled #29 from scratch, fixed the @todo and the deprecation strings, and the setUp:void change.

The patch looks smaller than #43 but that's because I have git configured to detect renames and copies.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3035174-renametrait-45.patch29.73 KB

Rerolled after #2664322: key_value table is only used by a core service but it depends on system install

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3035174-renametrait-48.patch29.64 KB

Reroll after #2879159: Some calls to assertEquals have expected/actual parameters reversed

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work 
+++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php
@@ -2,184 +2,20 @@
+@trigger_error(__NAMESPACE__ . '\SectionStorageTrait is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\layout_builder\SectionListTrait instead. See https://www.drupal.org/node/3091432', E_USER_DEPRECATED);
...
+ * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use

+++ b/core/modules/layout_builder/tests/src/Kernel/SectionStorageTestBase.php
@@ -2,200 +2,52 @@
+@trigger_error(__NAMESPACE__ . '\SectionStorageTestBase is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Tests\layout_builder\Kernel\SectionListTestBase instead. See https://www.drupal.org/node/3091432', E_USER_DEPRECATED);
...
+ * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use

the message should be about 9.3.0

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
StatusFileSize
new 3035174-50.patch29.57 KB
new interdiff_3035174_48-50.txt2.38 KB

Updated the patch in #48 addressing the changes specified in #49, thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Issue rationale makes sense. I read through the patch and the only changes are a copy from the old trait to the new trait, a matching copy in a test, and deprecation of the old code.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed aa47e8f and pushed to 9.3.x. Thanks!

  • catch committed 671433d on 9.3.x 
    Issue #3035174 by tim.plunkett, andypost, ankithashetty, jwwj,...
catch’s picture

catch
he/him
Primary language English
commented

... and published the CR.

Status: Fixed » Closed (fixed)

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