Problem/Motivation

As part of decoupling themes from Stable (parent issue: #3110855: Plan for removing dependency to Stable in Bartik/Seven/Claro/Umami) there will be tests that include comparing core's and Stable's template/CSS files (accounting for things like CSS image paths and @ingroup themeable.

Many of files that differ from core to Stable are only due to comments or indentation, and in almost every instance one of the versions is actually correct as opposed to something that is intentionally different based on the theme it resides in.

If these inconsequential differences are addressed, the tests can be implemented without having to account for dozens of noisy edge cases.

Proposed resolution

For each file, update either the Stable or core version so it matches its counterpart. Update the file that violates CS or a decision made in an issue.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3111729-9.patch24.15 KBbnjmnm
#9 interdiff_7-9.txt2.75 KBbnjmnm
#7 3111729-7.patch24.67 KBprabha1997
#7 interdiff_4-7.txt1.49 KBprabha1997
#4 interdiff_2-4.txt2.23 KBravi.shankar
#4 3111729-4.patch25.29 KBravi.shankar
#2 find-core-stable-differences.txt2.81 KBbnjmnm
#2 3111729-2.patch25.97 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
3111729-2.patch25.97 KB
find-core-stable-differences.txt2.81 KB

This patch updates 34 files that differ in core and Stable due to comment/indentation differences only. Many of them remove incorrect usage in Stable of @ingroup themeable or * Default theme implementaion

Also attached is a quick PHP script (extension changed to txt) I used to identify the differences. It requires xdiff to work.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work

Agreed this is a really good move to simplify things.
Patch mostly looks great. Only a few nits:

  1. +++ b/core/modules/language/templates/language-negotiation-configure-form.html.twig
@@ -9,7 +9,7 @@
- *     operates.
+operates.

    This seems wrong. 

  2. +++ b/core/themes/stable/css/toolbar/toolbar.module.css
@@ -79,7 +79,7 @@
-/* .toolbar-loading is required by Toolbar JavaScript to pre-render markup
+/* .toolbar-loading is required by toolbar JavaScript to pre-render markup

    Hrm, in this context, I think "Toolbar" is correct as it's a proper noun referring to a specific core module, right? 

  3. +++ b/core/themes/stable/templates/admin/language-negotiation-configure-form.html.twig
@@ -1,22 +1,23 @@
+ *   - description: A description for how the language negotiation type
+operates.

    Seems wrong again. 

  4. +++ b/core/themes/stable/templates/admin/language-negotiation-configure-form.html.twig
@@ -1,22 +1,23 @@
+ *   - attributes: A list of HTML attributes for the wrapper element.
+ * - children: Remaining form items for all groups.

    Probably out of scope, but this at first seemed like an indentation bug. Then I realized it's because -children is a top-level variable, not a child of -language_types. Wonder about moving -children to the top of the list, above -langugage_types? That'd both be alphabetical, and it'd avoid the possible confusion that -children is really top-level, not a mistakenly not-indented item under -language_types... 

  5. +++ b/core/themes/stable/templates/media-library/media-library-wrapper.html.twig
@@ -1,7 +1,7 @@
- * Default theme implementation of a container used to wrap the media library's
+ * Theme override of a container used to wrap the media library's
  * modal dialog interface.

    at least "modal" now fits on the line above. Maybe "dialog", too...

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3111729-4.patch25.29 KB
interdiff_2-4.txt2.23 KB

Here I have made changes as suggested in comment #3.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work

@ravi.shankar thanks for #4!
That only addresses #3 points 1, 3 and 5.
2 and 4 are still @todo.
At least 2 should be fixed. I think this means fixing the upstream core template, and leaving stable alone.
4 requires more discussion.

Cheers,
-Derek

prabha1997’s picture

prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commented
Assigned: Unassigned » prabha1997

I am working on this issue

prabha1997’s picture

prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commented
Assigned: prabha1997 » Unassigned
Status: Needs work » Needs review
FileSize
interdiff_4-7.txt1.49 KB
3111729-7.patch24.67 KB

Here I have made changes as suggested in comment #5.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work

@prabha1997: Thanks for #7, too!

As I said (twice), I think #3.4 requires more discussion. It is a little weird to have the first thing in the list something that says "Remaining form items..." -- remaining from what? ;) Probably we should leave it as it was originally. I was only asking for actual discussion about the merits of my review question, not new patches...

Meanwhile, for #3.2, patch #7 reverts the change to stable, but doesn't fix the upstream template to match. From #6: "I think this means fixing the upstream core template, and leaving stable alone." Patch #7 does 1 half of this, but not the other.

Sorry if I haven't been clear enough in my reviews and comments, and thanks for your contributions!
-Derek

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_7-9.txt2.75 KB
3111729-9.patch24.15 KB

I agree that #3.4 is out of scope here, but is worth looking into elsewhere. Created followup: #3112782: Potentially confusing docblock in language-negotiation-configure-form.html.twig and reverted the change applied here.

This also re-fixes #3.2 and corrects an indentation issue I spotted in status-messages.html.twig

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Reviewed & tested by the community

@bnjmnm: Thanks for the follow-up for #3.4. Now following the follow-up. ;)
Interdiff looks great.
All concerns are addressed.
RTBC.

Thanks, everyone!
-Derek

alexpott’s picture

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

Committed and pushed 74c0dd0aac to 9.0.x and 890f2fb81d to 8.9.x and a4fc70a12b to 8.8.x. Thanks!

As these are documentation or whitespace changes only I've backported this to 8.8.x to make any later bugfixes simpler to apply.

  • alexpott committed b17acb2 on 9.0.x 
    Issue #3111729 by bnjmnm, prabha1997, ravi.shankar, dww: Stable...

  • alexpott committed 890f2fb on 8.9.x 
    Issue #3111729 by bnjmnm, prabha1997, ravi.shankar, dww: Stable...

  • alexpott committed a4fc70a on 8.8.x 
    Issue #3111729 by bnjmnm, prabha1997, ravi.shankar, dww: Stable...

Status: Fixed » Closed (fixed)

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