Problem/Motivation

As mentioned in #3050389: [META] Remove dependency to Classy from core themes, Classy will be moved to contrib before Drupal 9 and we have to remove dependencies on Classy from all core themes. Part of removing these dependencies includes providing each theme their own copies of templates that previously belonged to Classy.

There's considerable potential for error when copying ~100 templates to four different themes, so we need a test that removes some of the uncertainty from the process.

Proposed resolution

Create a test that confirms that themes do not inherit templates from Classy, and that none of the templates they use include/extend Classy templates or attach Classy libraries.

The test will include a per-theme ignore-list of templates that do not need to be included as part of the test. At first, the ignore-lists will be very large, and will shrink with each template-copying issue filed. When a new template-copying issue is filed (for example, copy aggregator templates to Bartik), the first patch will be this test, but with aggregator's templates removed from Bartik's ignore list. This way, the test won't pass until the templates are properly copied.

Every Classy-inheriting core theme currently extends, includes or attaches something from Classy, so these tests will not pass at first.

Remaining tasks

Write the test, configured it to ignore templates that still need to be copied.

Create issues to address existing attach_library() calls to Classy libraries in Bartik/Seven/Claro/Umami, as these will cause the test created here to fail.

  • Umami calls {{ attach_library('classy/node') }} in node--article--full.html.twig, node--card--common.html.twig, node--card.html.twig, node.html.twig
  • Bartik calls {{ attach_library('classy/node') }} in node.html.twig
  • Claro calls {{ attach_library('classy/file') }} in file-link.html.twig
  • Seven calls {{ attach_library('classy/image-widget') }}in image-widget.html.twig

These library-copying followup issues can't be created until #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton is complete.

The Classy template include/require's should be addressed in the scope of this issue as this is where the test exists to verify they were done properly. This needs to be addressed with the following:

  • Bartik {% extends ""@classy/block/block--search-form-block.html.twig"" %} in block--search-form-block.html.twig
  • Bartik {% extends "@classy/block/block--system-menu-block.html.twig" %} in block--system-menu-block.html.twig
  • Bartik {% extends "@classy/content/page-title.html.twig" %} in page-title.html.twig
  • Bartik {% extends "@classy/misc/status-messages.html.twig" %} in status-messages.html.twig
  • Seven {% include '@classy/content-edit/image-widget.html.twig' %} in image-widget.html.twig
  • Claro {% extends '@classy/form/form-element-label.html.twig' %} in form-element-label.html.twig

The template copying can't happen until #3095713: Create classy directory with README, in the templates and css directories for all themes subtheming Classy is complete

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

CommentFileSizeAuthor
#32 3096349-27.again_.patch31.38 KBdww
#27 3096349-27.patch31.38 KBbnjmnm
#27 interdiff_25-27.txt1.1 KBbnjmnm
#25 3096349-25.patch31.42 KBbnjmnm
#25 interdiff_20-25.txt4.45 KBbnjmnm
#21 interdiff_18-20.txt1.74 KBbnjmnm
#21 3096349--20.patch31.16 KBbnjmnm
#18 3096349-18-REROLL.patch31.15 KBbnjmnm
#17 3096349-17.patch30.1 KBbnjmnm
#17 interdiff_12-17.txt3.13 KBbnjmnm
#12 3096349-12.patch27.57 KBbnjmnm
#12 interdiff_11-12.txt7.49 KBbnjmnm
#11 3096349-11__only-testing-includes_extends.patch26.22 KBbnjmnm
#11 interdiff_5-11.txt14.38 KBbnjmnm
#11 Remove_Classy_includesExtends__before-after.zip674.59 KBbnjmnm
#10 3096349__seven-claro__extend-fails.patch17.77 KBbnjmnm
#10 3096349__bartik_search-form-block-FAIL.patch18.27 KBbnjmnm
#10 3096349__bartik_block--system-menu-block-FAIL.patch18.27 KBbnjmnm
#10 3096349__bartik_page-title-FAIL.patch18.27 KBbnjmnm
#10 3096349__bartik_status-messages-FAIL.patch18.27 KBbnjmnm
#5 3096349-5.patch14.7 KBbnjmnm
#5 interdiff_2-5.txt10.03 KBbnjmnm
#2 3096349--2.patch16.5 KBbnjmnm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3096349--2.patch16.5 KB

This test will fail as it checks for templates in Claro/Bartik/Seven/Umami that include/extend/attach Classy assets. This issue can't be completed until #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton #3095713: Create classy directory with README, in the templates and css directories for all themes subtheming Classy are addressed, but not setting to postponed as the test itself can still be reviewed and moved forward without those issues being done yet.

Status: Needs review » Needs work

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

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: Create test for confirming Themes do not depend on Classy templates » [PP-2] Create test for confirming Themes do not depend on Classy templates
Status: Needs work » Postponed
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_2-5.txt10.03 KB
3096349-5.patch14.7 KB

Updated the test to skip the templates identified as identical to stable in #3098150: Add test coverage to ensure Classy templates that have identical versions in Stable are kept in sync

The test will still fail as it fails on templates including or extending Classy assets. Those will be changed in separate issues once #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton is done.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Related issues: +#3083275: [meta] Update tests that rely on Classy to not rely on it anymore

What's the relationship of this to #3083275: [meta] Update tests that rely on Classy to not rely on it anymore?

Should this also be blocked on that?

Or shall they basically happen in parallel?

Thanks,
-Derek

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

Thanks for pointing that out. If I'm correctly understanding #3083275: [meta] Update tests that rely on Classy to not rely on it anymore, there shouldn't be any dependencies between the issues and they can be done in parallel. This issue is for tests that confirm core themes do not depend on Classy templates, while #3083275 is to update existing tests so they do not rely on Classy.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Agreed. Added it as a child of #3050378: [meta] Replace Classy with a starterkit theme instead.

Thanks,
-Derek

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: [PP-2] Create test for confirming Themes do not depend on Classy templates » [PP-1] Create test for confirming Themes do not depend on Classy templates

#3098150: Add test coverage to ensure Classy templates that have identical versions in Stable are kept in sync has been committed.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue summary: View changes
FileSize
3096349__bartik_status-messages-FAIL.patch18.27 KB
3096349__bartik_page-title-FAIL.patch18.27 KB
3096349__bartik_block--system-menu-block-FAIL.patch18.27 KB
3096349__bartik_search-form-block-FAIL.patch18.27 KB
3096349__seven-claro__extend-fails.patch17.77 KB

As mentioned in the issue summary, there are six templates that include or extend Classy templates. Those include/extends should be addressed in the scope of this issue. Prior to doing that work, I'm providing these fail patches to show the the test catching this happening. One test for each bartik include/extend, and one that covers the occurrence in Claro and the occurrence in Seven.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
Remove_Classy_includesExtends__before-after.zip674.59 KB
interdiff_5-11.txt14.38 KB
3096349-11__only-testing-includes_extends.patch26.22 KB

This patch addresses the 6 templates in Classy-inheriting themes that include or extend @classy/ templates. They have been refactored to provide the same functionality without the presence of a Classy template.

The library related assertions in ThemesNotUsingClassyTemplatesTest are commented out in this patch so the testgroups can perform the extend/include assertions on each template without an expected attach_library() fail preventing completion.

A zip of before/after screenshots of each template in use is attached, with no change, and the markup was compared as well:

Bartik: block--search-form-block.html.twig

No change

<div class="search-block-form block block-search container-inline" data-drupal-selector="search-block-form" id="block-bartik-search" role="search">
	<h2>Search</h2>
	<div class="content container-inline">
		<form accept-charset="UTF-8" action="/search/node" class="search-form search-block-form" id="search-block-form" method="get" name="search-block-form">
			<div class="js-form-item form-item js-form-type-search form-type-search js-form-item-keys form-item-keys form-no-label">
				<label class="visually-hidden" for="edit-keys">Search</label> <input class="form-search" data-drupal-selector="edit-keys" id="edit-keys" maxlength="128" name="keys" size="15" title="Enter the terms you wish to search for." type="search" value="">
			</div>
			<div class="form-actions js-form-wrapper form-wrapper" data-drupal-selector="edit-actions" id="edit-actions">
				<input class="search-form__submit button js-form-submit form-submit" data-drupal-selector="edit-submit" id="edit-submit" type="submit" value="Search">
			</div>
		</form>
	</div>
</div>

Bartik: block--system-menu-block.html.twig

No change

<nav aria-labelledby="block-bartik-account-menu-menu" class="block block-menu navigation menu--account" id="block-bartik-account-menu" role="navigation">
	<h2 class="visually-hidden" id="block-bartik-account-menu-menu">User account menu</h2>
	<div class="content">
		<div class="menu-toggle-target menu-toggle-target-show" id="show-block-bartik-account-menu"></div>
		<div class="menu-toggle-target" id="hide-block-bartik-account-menu"></div><a class="menu-toggle" href="#show-block-bartik-account-menu">Show — User account menu</a> <a class="menu-toggle menu-toggle--hide" href="#hide-block-bartik-account-menu">Hide — User account menu</a>
		<ul class="clearfix menu">
			<li class="menu-item">
				<a data-drupal-link-system-path="user" href="/user">My account</a>
			</li>
			<li class="menu-item">
				<a data-drupal-link-system-path="user/logout" href="/user/logout">Log out</a>
			</li>
		</ul>
	</div>
</nav>

Bartik page-title.html.twig

No change
<h1 class="title page-title"><span class="field field--name-title field--type-string field--label-hidden" property="schema:name">Cool image</span></h1>

Bartik status-messages.html.twig

No change

<div data-drupal-messages="">
	<div class="messages__wrapper layout-container">
		<div aria-label="Status message" class="messages messages--status" role="contentinfo">
			<h2 class="visually-hidden">Status message</h2>Article <em class="placeholder"><a href="/node/2" hreflang="en">Cool image</a></em> has been updated.
		</div>
	</div>
</div>

Seven image-widget.html.twig

No change

<div class="image-widget js-form-managed-file form-managed-file clearfix">
	<div class="image-preview"><img alt="" class="image-style-thumbnail" data-drupal-selector="edit-field-image-0-preview" height="66" src="/sites/default/files/styles/thumbnail/public/2019-12/president-squid.PNG?itok=HfVpGPlv" typeof="foaf:Image" width="100"></div>
	<div class="image-widget-data">
		<div class="js-form-item form-item js-form-type-textfield form-type-textfield js-form-item-field-image-0-alt form-item-field-image-0-alt">
			<label class="js-form-required form-required" for="edit-field-image-0-alt">Alternative text</label> <input aria-describedby="edit-field-image-0-alt--description" aria-required="true" class="form-text required" data-drupal-selector="edit-field-image-0-alt" id="edit-field-image-0-alt" maxlength="512" name="field_image[0][alt]" required="required" size="60" type="text" value="squid">
			<div class="description" id="edit-field-image-0-alt--description">
				Short description of the image used by screen readers and displayed when the image is not loaded. This is important for accessibility.
			</div>
		</div><input data-drupal-selector="edit-field-image-0-fids" name="field_image[0][fids]" type="hidden" value="3"> <span class="file file--mime-image-png file--image" data-drupal-selector="edit-field-image-0-file-3-filename"><a href="http://drupal.test/sites/default/files/2019-12/president-squid.PNG" type="image/png; length=1715467">president-squid.PNG</a></span> <span class="file-size">(1.64 MB)</span> <input data-drupal-selector="edit-field-image-0-width" name="field_image[0][width]" type="hidden" value="1200"> <input data-drupal-selector="edit-field-image-0-height" name="field_image[0][height]" type="hidden" value="787"> <input data-drupal-selector="edit-field-image-0-display" name="field_image[0][display]" type="hidden" value="1"> <input class="button js-form-submit form-submit" data-drupal-selector="edit-field-image-0-remove-button" formnovalidate="formnovalidate" id="edit-field-image-0-remove-button" name="field_image_0_remove_button" type="submit" value="Remove">
	</div>
</div>

Claro form-element-label.html.twig

No change
<label class="form-item__label js-form-required form-required" for="edit-title-0-value">Title</label>

Another patch is on the way with expanded documentation based on feedback from a sibling issue: #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_11-12.txt7.49 KB
3096349-12.patch27.57 KB

Updated docs and removed a few bits of unnecessary code. The tests will still not pass until #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton is committed and theme-specific versions of the Classy libraries are created so the templates can library_attach() a non-Classy library. All test fails should be due to library use.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: [PP-1] Create test for confirming Themes do not depend on Classy templates » Create test for confirming Themes do not depend on Classy templates
Status: Postponed » Needs review

#3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton has landed! 🎉

Status: Needs review » Needs work

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

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Title: Create test for confirming Themes do not depend on Classy templates » [PP-?] Create test for confirming Themes do not depend on Classy templates

This is still postponed due #3106600: Decouple Classy libraries from Umami and some not-yet-created issues. There are overridden templates in all four Classy-inheriting themes that call attach_library() to Classy libraries, and these need to be changed to non-Classy equivalents before the tests will pass.

Once #3106600: Decouple Classy libraries from Umami is committed, we'll have an established the procedure for copying Classy libraries, and it may be possible to create an issue for making non-Classy copies of just the libraries required to get the tests in this issue passing. This would allow this issue to move forward without waiting for the full library copying to happen on all four themes.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs work » Postponed

I misunderstood the purpose of this issue. We've unblocked decoupling libraries, but not templates, so this issue needs to wait until all the library decoupling is complete. (However, we can get meaningful work in the meanwhile.)

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_12-17.txt3.13 KB
3096349-17.patch30.1 KB

This addresses Umami's attach_library() calls to Classy libraries, which is possible now that #3106600: Decouple Classy libraries from Umami is committed and there are Umami-specific versions of the Classy libraries.
Will make similar changes as the library decoupling issues for Seven, Claro and Bartik are committed.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Postponed » Needs review
FileSize
3096349-18-REROLL.patch31.15 KB

Rerolled and addressed the remaining attach_library() calls preventing the tests from passing. Interdiff not particularly easy due to the nature of the reroll but hopefully not a problem here since this hasn't been reviewed by anyone yet.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Title: [PP-?] Create test for confirming Themes do not depend on Classy templates » Create test for confirming Themes do not depend on Classy templates
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs followup 
  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+  protected $templatesSkippableBecauseIdenticalToStable = [

    This will allow us to decouple from Classy but not from Stable. Let's open follow-up also for planning decoupling from Stable. 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+        preg_match_all('/attach_library\(\'classy\/.+\'\)/', $template_contents, $classy_extend_library_matches);

    Should we add double quotes as a valid character to the regex as an extra precaution? 

  3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+        // Confirm template does not come from Classy.
+        $this->assertFalse($info['theme path'] === 'core/themes/classy', "$theme is inheriting $template_name from Classy.");

    Should we check for this condition first? It seems more logical to me since that way we
    don't have to check the extends|include and attach_library from Classy templates.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3096349--20.patch31.16 KB
interdiff_18-20.txt1.74 KB

This takes care of #20 2-3. Will address the followup requested in #1 after I've had the chance to discuss this and clarify a few details.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue tags: -Needs followup

Added followup #3110855: Plan for removing dependency to Stable in Bartik/Seven/Claro/Umami

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

This looks great. I'm tempted to RTBC it. A few minor nits/?s:

  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {

    a) Tiny nit: === instead of ==?

    b) Maybe this would be more readable with each || whatever clause on a newline? 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+        $this->assertEmpty($classy_extend_include_matches[0], "The template: '$template_name'' in the theme: '$theme' includes or extends a Classy template.");

    Looks like double ' after $template_name in the assert message. 

  3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+   * Data provider.

    Are we supposed to say "Data provider for ..."? 

  4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -0,0 +1,513 @@
+   * @return array
+   *   Test cases.

    Do we need to document the keys? 

  5. +++ b/core/themes/bartik/templates/block--search-form-block.html.twig
@@ -1,23 +1,48 @@
+ * - plugin_id: The ID of the block implementation.
+ * - label: The configured label of the block if visible.
+ * - configuration: A list of the block's configuration values, including:
+ *   - label: The configured label for the block.
+ *   - label_display: The display settings for the label.
+ *   - provider: The module or other provider that provided this block plugin.
+ *   - Block plugin specific settings will also be stored here.
  * - content: The content of this block.
  * - content_attributes: A list of HTML attributes applied to the main content
+ * - attributes: A list HTML attributes populated by modules, intended to
+ *   be added to the main container tag of this template. Includes:
+ *   - id: A valid HTML ID and guaranteed unique.
+ * - title_attributes: Same as attributes, except applied to the main title
  *   tag that appears in the template.
+ * - title_prefix: Additional output populated by modules, intended to be
+ *   displayed in front of the main title tag that appears in the template.
+ * - title_suffix: Additional output populated by modules, intended to be
+ *   displayed after the main title tag that appears in the template.

    Yay for adding these! I find accurate docs in twig templates incredibly valuable, and sometimes missing from core patches. Huzzah.

It's been a long day, and my eyes are glazing over trying to closely review the actual template changes included in here. But everything else looks great.

Thanks!
-Derek

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work 
+++ b/core/themes/bartik/templates/block--search-form-block.html.twig
@@ -1,23 +1,48 @@
+  'block',
+  'block-search',
+  'container-inline',
+]

+++ b/core/themes/bartik/templates/block--system-menu-block.html.twig
@@ -1,21 +1,66 @@
+  'block',
+  'block-menu',
+  'navigation',
+  'menu--' ~ derivative_plugin_id|clean_class,
+]

+++ b/core/themes/claro/templates/form-element-label.html.twig
@@ -2,11 +2,25 @@
+  'form-item__label',
+  title_display == 'after' ? 'option',
+  title_display == 'invisible' ? 'visually-hidden',
+  required ? 'js-form-required',
+  required ? 'form-required',
+]

Piling up on the nitpicks, these lines should be intended with two more spaces.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_20-25.txt4.45 KB
3096349-25.patch31.42 KB

This patch addresses all the good details spotted in#23 & #24.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented 
+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemesNotUsingClassyTemplatesTest.php
@@ -98,7 +98,12 @@ protected function installAllModules() {
+      if ($module->origin !== 'core'
+        || !empty($module->info['hidden'])
+        || $module->status === TRUE
+        || $module->info['package'] === 'Testing'
+        || $module->info['package'] === 'Core (Experimental)'

Nitpick : according to Drupal coding standards, this isn't allowed: https://www.drupal.org/docs/develop/standards/coding-standards#linelength 🔬👁

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff_25-27.txt1.1 KB
3096349-27.patch31.38 KB

For #26

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/themes/bartik/templates/status-messages.html.twig
@@ -16,13 +15,46 @@
+ * - attributes: HTML attributes for the element, including:
+ *   - class: HTML classes.

This seems inaccurate but since this has been just copied from the Classy template, it's probably fine for now.

I manually removed aggregator-feed from the templates to be skipped and the tests failed, which means that the tests work as expected.

The last submitted patch, 17: 3096349-17.patch, failed testing. View results

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Related issues: +#1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines

Re: #26 / #23.1b: Ugh, I'm so sorry. I've mistaken how I think it should be for what we've actually agreed on. ;) I forgot that #1539712 is still open. Whoops. :( See #1539712-86: [policy, no patch] Coding standards for breaking function calls and language constructs across lines (and some following +1s/comments) for thoughts on why I believe 23.1b is better (easier to read and maintain in the future). Thanks @bnjmnm for implementing it and sorry you had to undo it. ;) Thanks @lauriii for reminding me what's the current reality, not my fantasy. ;)

All that said, +1 to RTBC. This is ready. Don't know what the bot is thinking with #29...

Thanks!
-Derek

The last submitted patch, 17: 3096349-17.patch, failed testing. View results

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
FileSize
3096349-27.again_.patch31.38 KB

Bot seems to be confused, and is re-testing #17 for no apparent reason. Re-uploading #27 in the hopes this helps.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +Drupal 9.0.0-beta1 requirement

  • Gábor Hojtsy committed 9a5df17 on 9.0.x 
    Issue #3096349 by bnjmnm, dww, lauriii: Create test for confirming...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks for the thorough test coverage. Landed!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Also in 8.9, not sure why it did not show up here. See https://git.drupalcode.org/project/drupal/commit/e3221b6ddc314669f57d5dc...

  • Gábor Hojtsy committed e3221b6 on 8.9.x 
    Issue #3096349 by bnjmnm, dww, lauriii: Create test for confirming...

Status: Fixed » Closed (fixed)

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