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 Classy's preprocess functions, provided they are relevant to the theme.
These are the three functions that need to be decoupled in Claro, Seven, Bartik and Umami
function classy_preprocess_links__media_library_menu(array &$variables) {
foreach ($variables['links'] as &$link) {
$link['link']['#options']['attributes']['class'][] = 'media-library-menu__link';
}
}
function classy_form_alter(array &$form, FormStateInterface $form_state, $form_id) {
$form_object = $form_state->getFormObject();
if ($form_object instanceof ViewsForm && strpos($form_object->getBaseFormId(), 'views_form_media_library') === 0) {
$form['#attributes']['class'][] = 'media-library-views-form';
}
}
function classy_preprocess_image_widget(array &$variables) {
$data = &$variables['data'];
if (isset($data['preview']['#access']) && $data['preview']['#access'] === FALSE) {
unset($data['preview']);
}
}
Proposed resolution
For each Classy inheriting theme, determine if each Classy preprocess function is relevant to the theme. If it is relevant, copy it to the theme and make changes where necessary so it functions identically and does not result in regressions or redundant functionality such as adding the same class multiple times.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 514 bytes | lauriii |
#22 | 3109287-22.patch | 14.36 KB | lauriii |
#21 | 3109287-21.patch | 14.34 KB | bnjmnm |
#21 | interdiff_19-21.txt | 3.21 KB | bnjmnm |
#19 | interdiff_17-19.txt | 2.33 KB | bnjmnm |
Comments
Comment #2
bnjmnmIn addition to the patch, there are screenshots of a manual review. To perform this review, I commented all the functions from classy.theme so they became the responsibility of Seven/Claro/Bartik/Umami. For
form_alter
andlinks__media_library_menu
, I confirmed the classes added by the functions are present. Forimage_widget
, it's an Xdebug breakpoint confirming the function is called.Comment #3
bnjmnmComment #4
bnjmnmComment #5
dwwSummary 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:
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
Comment #6
bnjmnm@dww +1 to all the points in #5. Setting this to "Needs work" so this doesn't get additional reviews until I've created some followups and added some @todo items in the patch.
Comment #7
bnjmnmCreated these issues:
#3110137: Remove Classy from core
#3110132: Could media library preprocess/alters in Classy be moved to module or another centralized place?
Many added instances of @todo and @see. I'd like it if it were a bit more readable, but not sure how to do that in a way that adheres to CS. Suggestions welcome. If there was agreement in #3110132: Could media library preprocess/alters in Classy be moved to module or another centralized place? that those functions could be moved to media library that could potentially make this entire issue unnecessary...
Comment #8
dwwSweet, 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
Comment #9
bnjmnmJust as well that I neglected to upload the patch in #7, as #3110132: Could media library preprocess/alters in Classy be moved to module or another centralized place? provided enough context to move
classy_preprocess_image_widget
out of Classy and into a module.That same issue also directed me to the discussions that informed the policy resulting in Media Library-specific functions in classy.theme. While I believe this is worth revisiting due to changes in Drupal since these discussions took place (core is starting to offer more opinionated modules -- and users like them), it's not a change that could be implemented casually.
The added comments are a bit tamer than the forgot-to-upload patch in #7, but there are still some stacked @todo/@see instances that I'm happy to entertain suggestions for making more readable.
Comment #10
bnjmnmComment #11
Gábor HojtsyTagging.
Comment #12
lauriiiIt 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.Comment #13
bnjmnmRe: #12
The followup referenced by the @todo items exists, #3110137: Remove Classy from core, but I added a task to the issue summary specifically to address @todo items referencing the issue.I created a followup, and added @todo items referencing it: #3114318: Centralize image widget's preview access functionality
Comment #14
lauriiiAny 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?
Comment #15
bnjmnmThe test only requires a trifle more effort than adding a comment to classy.theme saying "DONT UPDATE THIS ANYMORE", so seems like a good idea. Added it.
Comment #16
dwwLove 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.
Comment #17
bnjmnmGood call in #16, the test is a good final line of defense but that's an easy way to make it so the test results aren't the only source of info.
Comment #18
dwwRe: #17 Thanks!
Hate to do this, but:
double "the the"
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. ;)
Comment #19
bnjmnmAddressed the duplicate "the" instances from #17, glad that goof was spotted.
I opted to add the warning twice because it's an appropriate addition to @file
, but I was also concerned that it could be overlooked. That block is often ignored, collapsed by default in PHPStorm, and potentially out of view at the bottom of the file -- the place new functionality would likely be added. If this were a file planned to have continued development I'd be wary of two instances of the same warning as they could potentially go out of sync and add noise. In this case, however, I think it's beneficial to boost the visibility letting contributors know that this is probably not the correct file to work on.
Comment #20
lauriiiThe 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.
Comment #21
bnjmnmRe #20 -- moved the test to a dedicated class. Expanding the class documentation had too high a risk of making it incoherent.
Comment #22
lauriiiAdded @group annotation which will hopefully fix the tests.
Comment #23
lauriiiConfirmed 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.
Comment #25
Gábor HojtsyLooks good, thanks all!
Comment #26
Gábor HojtsyIntentionally 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.