Don't try to include Media CSS library if Media Entity 1.x is being used

Path #4 is working.

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)

Patch looks good to me.

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

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.

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? :)

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

OK then :)
Thanks!

RTBC for the 8.4 patch.

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.

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).

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.

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

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

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?

+++ 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.

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.

+++ 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

#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)

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

Thanks for clarifying @seanB

Committed as 88f92b2 and pushed to 8.4.x.

This is not in 8.5.x?

The followup did not make it into 8.5.x / 8.6.x - #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead - so we still have the problem there. Let's commit this to 8.6.x and 8.5.x and then remove the code in #2916786: Stop adding specific CSS to Media form and use ContentEntityForm regions instead if we can. I'm not sure why we only commit this to 8.4.x - doesn't really make sense to me.

I will commit this in 24hrs unless someone gives a good reason as to why not.

Ok so the 8.6.x alpha got in the way and it means that we can't backport this to 8.5.x.

Committed and pushed 3764218018 to 8.7.x and e0ea889653 to 8.6.x. Thanks!