Problem/Motivation

#2938132: Ship layouts that make sense with Layout Builder's concept of sections added new layouts to Layout Builder, two of which have configuration.
There is no config schema defined for that configuration.
This should have been caught by tests, but was not

Proposed resolution

Add config schema
Fix the tests

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3029061-schema-15-interdiff.txt4.9 KBtim.plunkett
#15 3029061-schema-15-PASS.patch4.58 KBtim.plunkett
#15 3029061-schema-15-FAIL.patch3.86 KBtim.plunkett
#2 3029061-schema-2-PASS.patch1.77 KBtim.plunkett
#2 3029061-schema-2-FAIL.patch1.05 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
3029061-schema-2-FAIL.patch1.05 KB
3029061-schema-2-PASS.patch1.77 KB

There was existing test coverage of this plugin, but the configuration was never actually saved in config, only in tempstore.

The last submitted patch, 2: 3029061-schema-2-FAIL.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

So long as this is committed before 8.7.0-alpha1, it doesn't need an upgrade path or anything like that, since this change was just added in 8.7.x a couple weeks ago.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented 
+++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
@@ -64,3 +64,16 @@ inline_block:
+layout_builder_multi_width:

I'm not sure how I feel about the name of this data type. How about layout_plugin.layout_builder_multi_width?

Other than that, looks good and RTBC from me.

tim.plunkett’s picture

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

To me that implies it's owned by a module called layout_plugin. But honestly the name does not matter much to me, I just thought it would be better to not duplicate the info shared between the two plugins.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Okay, fair enough. It's not really a big deal.

  • xjm committed f192f65 on 8.7.x 
    Issue #3029061 by tim.plunkett, phenaproxima: New layouts are missing...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.7.x. Thanks!

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented

This introduced random test failure in HEAD:
- https://www.drupal.org/pift-ci-job/1185181
- https://www.drupal.org/pift-ci-job/1185177
- https://www.drupal.org/pift-ci-job/1185180

1) Drupal\Tests\layout_builder\FunctionalJavascript\TestMultiWidthLayoutsTest::testWidthChange
WebDriver\Exception\UnknownError: unknown error: Element ... is not clickable at point (60, 571). Other element would receive the click: ...
  (Session info: headless chrome=62.0.3202.94)
  (Driver info: chromedriver=2.33.506092 (733a02544d189eeb751fe0d7ddca79a0ee28cce4),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)

/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/Exception.php:155
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:157
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:218
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/Container.php:224
/var/www/html/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:775
/var/www/html/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:763
/var/www/html/vendor/behat/mink/src/Element/NodeElement.php:153
/var/www/html/vendor/behat/mink/src/Element/TraversableElement.php:73
/var/www/html/core/modules/layout_builder/tests/src/FunctionalJavascript/TestMultiWidthLayoutsTest.php:106
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

I think this might have introduced a new fail on slower environments - see https://www.drupal.org/pift-ci-job/1185177 - both PHP 5.5 jobs have failed since this was committed.

  • alexpott committed 6f7fe9b on 8.7.x 
    Revert "Issue #3029061 by tim.plunkett, phenaproxima: New layouts are...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Fixed » Needs work

Unfortunately this is back to needs work. I'm wondering why \Drupal\KernelTests\Config\DefaultConfigTest didn't catch this.

tim.plunkett’s picture

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

Because nothing in core ships with either of the two plugins configured.

I'll work on this tomorrow.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
3029061-schema-15-FAIL.patch3.86 KB
3029061-schema-15-PASS.patch4.58 KB
3029061-schema-15-interdiff.txt4.9 KB

Switching from a functional test to a kernel test.

The last submitted patch, 15: 3029061-schema-15-FAIL.patch, failed testing. View results

tim.plunkett’s picture

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

Also note that now we get the DefaultConfigTest failure in addition to the explicit fail, which is great.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Okay, well...cool!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Adding 5.5 and 5.6 tests to be safe.

The 5.6 test has been segfaulting for weeks now and that was not introduced by this patch.

  • xjm committed 1d83b5f on 8.7.x 
    Issue #3029061 by tim.plunkett, xjm, phenaproxima, alexpott, tacituseu:...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.7.x. Thanks for catching the test regression!

Status: Fixed » Closed (fixed)

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