Problem/Motivation

The seven theme tries to include some CSS inside a form alter that affects all media forms:

/**
 * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\media\MediaForm.
 */
function seven_form_media_form_alter(&$form, FormStateInterface $form_state) {
  // @todo Revisit after https://www.drupal.org/node/2892304 is in. It
  // introduces a footer region to these forms which will allow for us to
  // display a top border over the published checkbox by defining a
  // media-edit-form.html.twig template the same way node does.
  $form['#attached']['library'][] = 'seven/media-form';
}

However, it is possible that a site is using Media Entity 1.x instead of core Media, so this library won't be available in that case.

Proposed resolution

Only include the core CSS library if we are using core Media, not contrib.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

woprrr created an issue. See original summary.

woprrr’s picture

Status: Active » Needs review
FileSize
1.22 KB

Here the patch to solve this problem and unsure we are in MediaForm (CORE) context only to attach this.

woprrr’s picture

Issue summary: View changes
woprrr’s picture

OOps !! forget MediaForm use. sorry for noise.

MichaelB’s picture

Path #4 is working.

marcoscano’s picture

Title: Media & seven theme dependencies » Don't try to include Media CSS library if Media Entity 1.x is being used
Issue summary: View changes
Issue tags: +Media Initiative
FileSize
1.45 KB
820 bytes
2.03 KB

This issue comes from #2863431: Change "Save and keep un-/published" buttons for media module and I can confirm without the patch sites using Media Entity 1.x + Drupal 8.4.x will have warnings in the watchdog:
User warning: The following theme is missing from the file system: media in drupal_get_filename() (line 248 of /var/www/html/core/includes/bootstrap.inc)

In the new patch I have just reworded the comment a little bit to make it more explicit.

Also, the issue previously mentioned on the @todo (#2892304: Introduce footer region to ContentEntityForm) landed in 8.5, so we should remove all this stuff in 8.5 now. Do we need another issue for that or can we use this same one? In any case, I'm uploading a basic patch against 8.5.x, only removing the cruft and adding the checkbox to the footer region. I believe more work could follow to have the twig template done, but this could probably live in a follow-up? (not sure if one aiming to improve media forms exists already)

Manuel Garcia’s picture

Patch looks good to me.

As far as the @todo for the footer region, I think doing it as a new issue makes sense.

seanB’s picture

Nice work! I also think this issue should just contain the fix for 8.4, and we should create a separate issue to do the cleanup shown in the 8.5 patch. I created #2916784: Remove temporary seven theme workaround for media forms to fix this.

marcoscano’s picture

Thanks @Manuel Garcia!

@seanB you are fast man!!! :) I was going to post that I had created #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead, but you were quicker. Which one do we close? :)

seanB’s picture

Yours has a patch, I'll close mine :)

marcoscano’s picture

seanB’s picture

Status: Needs review » Reviewed & tested by the community

RTBC for the 8.4 patch.

xjm’s picture

Version: 8.4.0 » 8.4.x-dev
Status: Reviewed & tested by the community » Needs review

I think the approach here makes sense -- however, I'm not sure it's safe to backport this to a patch release. Someone could be relying on the library being there in Seven. What happens in particular when the library is missing with Media Entity 1.x?

Maybe we could commit the current patch to 8.5.x, but use an approach for 8.4.x that just fixes the alter hook internally to only fire if the Media module is installed? That would remove the issue when the contrib module is installed without risking disruption to production sites.

woprrr’s picture

Hii @xjm

I think that patch only fire on Media module (core) context because we check what instance of MediaForm are currently used. If we use MediaForm (core) the instance of MediaForm match with "Drupal\media\MediaForm" only not with "Drupal\media_entity\MediaForm" used by Media Entity (contrib).

But I understand your opinion for 8.4 branch, here the patch for 8.4

xjm’s picture

Yep, #14 is along the lines of what I was thinking, thanks!

Probably we could commit that patch to both branches, and then use #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead as the followup to remove it in 8.5.x. I've postponed the other issue for now.

Leaving at NR for someone else to review/+1 #14 so that I could potentially commit it (since it was my suggestion).

seanB’s picture

There is actually a media contrib module for D8, so sites that installed the Media contrib module will still have the problem when we only check if media is installed.

I think that is why it's safer to check the form instance.

woprrr’s picture

I'm agree @seanB to me InstanceOf object is more precise but is installed offert same results (I have tested again to be sure in two sites on production).

Leaving at NR for someone else to review/+1 #14 so that I could potentially commit it (since it was my suggestion).

@xjm +1 That sound great ! Soon as possible to merge it onto 8.4.

Ready to RTBC ? if @seanB accept usage of "is installed" for 8.4 only ?

idebr’s picture

The duplicate issue #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead fixed this issue by removing the dependency on the media/form library altogether, since there is no actual dependency strictly speaking. That approach would fix this issue and can be committed to both 8.4.x and 8.5.x

idebr’s picture

Issue tags: +Seven

I looked for an existing issue related to Seven, but could not find any. Should this not be in the Seven component?

woprrr’s picture

Hii @idebr,

This issue and patch associated are to unblock fast the situation and avoid all blockers for media / media_entity user.
Like @xjm say :

Probably we could commit that patch to both branches, and then use #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead as the followup to remove it in 8.5.x. I've postponed the other issue for now.

We need to merge it first and then remove completely this dependency if needed.

seanB’s picture

As explained in #16 I think the 2916741-8.4.x-6.patch patch is the one we should probably commit in stead of the #14 patch.
Are there any other concerns we need to address? Otherwise, we can probably go back to RTBC and unblock #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead for 8.5?

woprrr’s picture

As seen with @xjm on wednesday and today with @seanB on slack, the proposal would be to perform a double check to reduce the problems around this test. I get the variable before the test for questions of readability of the code.

What do you think of this approach ? @xjm

seanB’s picture

+++ b/core/themes/seven/seven.theme
@@ -167,9 +168,14 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
+  $media_is_enabled = \Drupal::moduleHandler()->moduleExists('media');
...
+  if ($media_is_enabled && $form_state->getFormObject() instanceof MediaForm) {

There is no real need for $media_is_enabled, so let's drop the extra variable.

Besides that, I think this addresses #13 and #16.

woprrr’s picture

@seanB this extra variable is only here to lisibility :/ this part of code this very long méthod call in condition make me sad and primary symptom of bad code smell. I can change that if you think this is unecessary but for these reasons I think we can add this variable and have better clarity of what we need to test here

1/ media is enable
2/ media is instanceof mediaType

woprrr’s picture

In any case I will not make a hard point the issue blocks too much so I change.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Now let's hope this is acceptable for 8.4 since it solves a really annoying error for sites using media_entity 1.x.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/seven/seven.theme
@@ -167,9 +168,13 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
+    // @todo Revisit after https://www.drupal.org/node/2892304 is in. It
+    // introduces a footer region to these forms which will allow for us to
+    // display a top border over the published checkbox by defining a
+    // media-edit-form.html.twig template the same way node does.

This issue is in. So we should revisit it here.

Thanks

seanB’s picture

Status: Needs work » Reviewed & tested by the community

#2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead does the proper cleanup, but we can't add this to 8.4 as per #13.

The patch in this issue is to fix the message below when using media 1.x for 8.4 only:
User warning: The following theme is missing from the file system: media in drupal_get_filename() (line 248 of /var/www/html/core/includes/bootstrap.inc)

larowlan’s picture

Added credit for @xjm and @seanB for reviews/mentoring.

Thanks for clarifying @seanB

  • larowlan committed 88f92b2 on 8.4.x
    Issue #2916741 by woprrr, marcoscano, seanB, xjm: Don't try to include...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +DrupalSouth 2017

Committed as 88f92b2 and pushed to 8.4.x.

Status: Fixed » Closed (fixed)

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