Decouple Classy-inheriting themes from Classy's preprocess functions

Summary sounds reasonable.
Test bot is happy (including no new CS violations).
Patch (mostly) looks good.

It's kinda a shame to duplicate this code in all these places, but that's required given a) by our own policy, we can't add these classes in core itself and require themes to do so themselves and b) are moving away from having a shared base theme for core themes. Therefore, there's no good place to put shared code anymore, so we have to live with copy/paste and repeating ourselves. If all core themes need/want to add these classes to the media library, I wonder if a) is actually true. Maybe the better patch would be adding the classes in the media library itself and then ripping all this crap out of classy in the first place... but that's not really a fight I want to have this morning. ;)

The other reason I hesitate to RTBC this is because of all these comments:

+++ b/core/themes/seven/seven.theme
@@ -214,6 +214,14 @@ function seven_form_alter(array &$form, FormStateInterface $form_state, $form_id
+    // This conditional exists because the media-library-views-form class is
+    // currently added by Classy, but Seven will eventually not use Classy as a
+    // base theme.
+    // @see classy_form_alter()

Can we open a postponed follow-up (for D10?) to un-do this conditional, add @todo to these comments, and include a @see pointing to the task issue? Seems like we're adding some technical debt here, and we should have a clear plan for removing it when the time is right.

Thanks!
-Derek

Sweet, thanks for opening those!

I thought #3110137: Remove Classy from core was duplicate with #3050378: [meta] Replace Classy with a starterkit theme but I see you envision it as a separate step. Updated some metadata and links accordingly.

I also replied at #3110132: Could media library preprocess/alters in Classy be moved to module or another centralized place?. Yay for that! Hope it works. ;)

Meanwhile, it sounds like you made changes here, but you didn't upload the new patch. Maybe it's not worth uploading nor reviewing until the dust settles on #3110132 but then we should call this postponed, not NR.

So, NW for either a) upload current patch or b) change title and status to officially postpone on #3110132 if you think that's the right strategy.

Thanks,
-Derek

Tagging.

It seems fine to add the conditional addition of the classes to the preprocess functions where needed. The patch already has @todo comments for removing the if statements. However, we don't have a great track record ensuring that @todo comments get addressed. Let's ensure that a follow-up exists for removing the if clauses, which should increase the likelihood of them being removed on time. 😉

I'm wondering if there would be a way to move classy_preprocess_image_widget to CSS. How about we copy the preprocess function to Seven and Claro in this issue, and open a follow-up where we decide whether it should be moved to the image module or moved to CSS? This way we can minimize any resistance on this issue.

Any thoughts on creating small test for ensuring that any new preprocess functions doesn't get added to Classy that would need to be taken into account in future?

Love the new test. ;)

However, I wonder if adding the comment (and updating the file hash for the test) would also be helpful. Not everyone runs tests locally, and it's a lot of churn for both the d.o test bots and issue reviewers/contributors to have folks posting patches that we know will fail. Let's both document they shouldn't change the file in the file itself, and have the test from #15 to prove it.

Re: #17 Thanks!

Hate to do this, but:

1. +++ b/core/themes/classy/classy.theme
   @@ -3,6 +3,10 @@
   + * will be deprecated in Drupal 9. Changes should instead be made in the the
   

   double "the the"

2. +++ b/core/themes/classy/classy.theme
   @@ -38,7 +42,18 @@ function classy_form_alter(array &$form, FormStateInterface $form_state, $form_i
   +
   +/**
   + * No changes that impact functionality should be made to this file, as Classy
   + * will be deprecated in Drupal 9. Changes should instead be made in the the
   + * core themes Bartik, Seven, Claro and/or Umami.
   + */
   

   Not clear from context why all this is here, but it seems duplicate with the @file comment.

   And if we do need it here, double "the the" again. ;)

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyDecoupledTest.php
@@ -14,7 +14,7 @@
+class ConfirmClassyDecoupledTest extends KernelTestBase {

The md5 based test is probably fine since we're not expecting to make lots of changes to the file. However, we should either move this to a new test class or update the class documentation to include the preprocess functions.

Added @group annotation which will hopefully fix the tests.

+++ b/core/themes/classy/classy.theme
@@ -38,7 +42,18 @@ function classy_form_alter(array &$form, FormStateInterface $form_state, $form_i
 function classy_preprocess_image_widget(array &$variables) {
   $data = &$variables['data'];
...
   if (isset($data['preview']['#access']) && $data['preview']['#access'] === FALSE) {
     unset($data['preview']);
   }
 }

Confirmed that this isn't needed by Bartik and Umami because I didn't document rationale for skipping them in #12. The reason this isn't needed is that this leads into rendering an empty span, which doesn't have any styling in those themes.

Other than that everything seems consistent. The test coverage and documentation should ensure that classy.theme doesn't get changed in a way that would lead these themes being out of sync.

Looks good, thanks all!

Intentionally only went to Drupal 9.0.x since the base theme mechanism is still in place in Drupal 8 and therefore the two versions share the same behaviour and the preprocess functions are not an API.