image-widget.html.twig references a non-existing classy/image-widget library

Hi!

The file indeed exists you can find it at:

core/themes/classy/css/components/image-widget.css

UPDATE: ohh you are right, the image-widget is not declared in the classy.libraries.yml, so this seems a bug.

This how image-preview looks right now:

And this is how it looks adding the library in the classy.library.yml file:

And I attach a patch with the code for add this library in the classy.library.yml file.

Thanks!

True, I was confused about how the libraries thing work, #5 looks great :-)

Committed/pushed to 8.1.x, thanks!

While this is a straight bugfix, it's potentially a visual change for a subtheme of classy, so I don't think it can go into a patch release.

If we were going to fix this in 8.0;x, I'd remove the dead CSS (or move it to Seven) and the reference in the template, then there's no change for subclassing themes.

I think that's the right fix in general so reverted from 8.1.x for now.

In this patch I moved the library to seven and removed the reference from classy

This can break themes because CSS that was mistakenly not included before is now suddenly included. If someone added their own CSS to account for it, that will suddenly cause a problem. It would likely be slight, but still.

I could also argue that since it creates any visual change, and would go in 8.1, that it could have a change record.

Agree with #17 and even also agree with 13 any of those approach is good to me.

Depending on the visual bugs this may cause I'd keep it in 8.0.x and let the committer decide. Probably just needs to go into Bartik as well but I think #15 patch is going in the right direction for this fix.

And 8.1 is almost out and this fix is not part of the core :-/

I will move it to "needs review" again at this point doesn't matter if should go at 8.1 or 8.0

Moving to 8.2.x because 8.1.x is out and this introduce a little visual change, also I updated the issue Summary.

The patch at number at #15 needed reroll. I have done that and tested it. It looks good. Thanks @gnuget!

Without patch:

With patch:

The tests have passed. Marking as RTBC.

Please don't rtbc your own patches. Someone else will need to review it.

Hi @davidhernandez. I just did the reroll. Since no code changes were made and I tested the patch I thought it was ok to mark the issued RTBC. I was not aware I could not mark the issue RTBC in such a case. Thanks for setting it back to needs review.

The reroll on #26 works fine :)

Thanks, a couple points:

1. diff --git a/core/themes/classy/css/components/image-widget.css b/core/themes/classy/css/components/image-widget.css
   deleted file mode 100644
   

   I think we should probably just leave this file and potentially just add a comment to say that it's not being loaded by Classy. Someone could be referencing this from their theme, maybe even fixing the bug in their own theme. I think that is possible but I might be wrong.

2. +++ b/core/themes/seven/templates/content-edit/image-widget.html.twig
   @@ -0,0 +1,24 @@
   +<div{{ attributes }}>
   +    {% if data.preview %}
   +        <div class="image-preview">
   +            {{ data.preview }}
   +        </div>
   +    {% endif %}
   +    <div class="image-widget-data">
   +        {# Render widget data without the image preview that was output already. #}
   +        {{ data|without('preview') }}
   +    </div>
   +</div>
   

   Indentation in this file looks off, 4 spaces instead of 2. It should have the same indentation as Classy's image-widget.html.twig.

Thanks for your review @Cottser. The new patch also attaches the 'seven/image-widget' library from image-widget.html.twig as the Classy template did.

Test patch #32 , works fine. attach screenshots.

1. +++ b/core/themes/classy/css/components/image-widget.css
   @@ -1,6 +1,10 @@
   + * This file is not being loaded by Classy.
   + *
   + * @see https://www.drupal.org/node/2641380
   

   This is very unsatisfactory. We should at least say why it is not being loaded, and therefore why this file then still exists.

2. +++ b/core/themes/seven/templates/content-edit/image-widget.html.twig
   @@ -0,0 +1,24 @@
   +<div{{ attributes }}>
   +  {% if data.preview %}
   +    <div class="image-preview">
   +      {{ data.preview }}
   +    </div>
   +  {% endif %}
   +  <div class="image-widget-data">
   +    {# Render widget data without the image preview that was output already. #}
   +    {{ data|without('preview') }}
   +  </div>
   +</div>
   

   This is identical to Classy's template!

   This should therefore use {% extends … %}, and just do the {{ attach_library(…) }}.

Hi! Wim Leers

Thanks for your review!

2. We cannot use {% extends .. %} because as far as I know with extends we must overwrite a theme parent block and classy's image-widget.html.twig template have none of those, I tried using the usekeyword instead of extends but I got this error:

wig_Error_Runtime: Template "@classy/content-edit/image-widget.html.twig" cannot be used as a trait

So I'm not sure how to do this without copy/pasting the entire template and adding there the library.

miss I something?

Thanks!

You don't need to use Twig blocks at all. You can do things like:

{% extends "image.html.twig" %}
{%
set classes = [
  style_name ? 'image-style-' ~ style_name|clean_class,
]
%}
{% set attributes = attributes.addClass(classes) %}

Hi, Wim Leers.
But we dont have in seven image.html.twig, we need create image.html.twig. Or you mean include from another place?

That was just an example. Here, it would be:

core/themes/seven/templates/content-edit/image-widget.html.twig would contain:

{% extends "@classy/content-edit/image-widget.html.twig" %}
{{ attach_library('seven/image-widget') }}

I think best solution, use #5 patch. Just include image-widget.css in classy. Classy
сomponents works in seven too. And we dont need move to seven additional files.

Hi @Wim Leers.

I already tried what you suggested (I just did it again just to make sure) and this is the error I got:

The website encountered an unexpected error. Please try again later.

Twig_Error_Syntax: A template that extends another one cannot have a body in "core/themes/seven/templates/content-edit/image-widget.html.twig" at line 14. in Twig_Parser->filterBodyNodes() (line 379 of vendor/twig/twig/lib/Twig/Parser.php).

I investigated that error and I found out to this error is related to what I mentioned at #37 I think your example at #38 is working because doesn't print anything in the image.html.twig theme but not sure.

[update] I tried using {% set something %} and yes the error is gone so the problem is when we try to print something outside of a block I don't think the {% extends %} approach is gonna work for this particular case. :(

My bad. This is what that should look like:

{% extends "@classy/content-edit/image-widget.html.twig" %}
{% attach_library('seven/image-widget') %}

Sorry for being a pain.

I tried what you suggested and I got a new error:

Twig_Error_Syntax: Unknown "attach_library" tag in "core/themes/seven/templates/content-edit/image-widget.html.twig" at line 14. in Twig_Parser->subparse() (line 178 of vendor/twig/twig/lib/Twig/Parser.php).

And checking the documentation https://www.drupal.org/developing/api/8/assets

Attaching a library in a twig template

You can also attach a library in a twig template by using the attach_library() twig function. So in any *.html.twig:

{{ attach_library('contextual/drupal.contextual-links') }}
<div>Some markup {{ message }}</div>

It seems to actually we must use {{ extends }} instead of{% extends %}

thanks for all your help and time.

David.

And while Wim Leers doesn't be happy with the fix this still will need work.

{% extends %} and attach_library dont work together in one file core\themes\seven\templates\content-edit\image-widget.html.twig So I decided include image-widget.css in classy theme. And add in seven dependence from classy.

If the only thing in the template is the extends line, then there is no need for the template. I'm wondering why the attach_library didn't work there. We should figure that out.

Hi @Chernous_dn

The initial bug is to the image-widget.css is missing in the classy theme, the approach to you took at #46 was already explorer at #5, We didn't committed this approach because:

• This will change the UI and that is a major change.
• Maybe other themes already fixed this issue in their own subtheme so we cannot just remove the attach_library in the classy image-widget.html.twig
• You cannot add just the file in the library's file in the classy theme because classy already use it and it could break subthemes.

The alternative suggested at #31 is:

• Left the css file and the attach_library in the classy image-widget.html.twig, It means, we must leave the bug in case a subtheme already fixed it (adding the css in the libraries yml file) and just leave a comment in the CSS file explaining why that file is not used at all in the classy theme and why we leave it, so here we must explain to the file was never added in the libraries classy theme (bug!) but it could be added in a subtheme so we cannot just remove it and we cannot just added it in the classy libraries file either because that could break other subthemes.
• We cannot change classy but we can change Seven, so we can just add the library in seven and this should be fixed but we have the problem exposed at #35 where we must copy the entire TPL and just ad the library which is not desired, so basically we are stuck here.

Hi David.

What do you mean with the attach_library didn't work there?

He said in #46 they didn't work in the same file.

He said in #46 they didn't work in the same file.

I explained why at: #37.

Thanks for your time on this :-)

We can do more in an extended template than override a parent block. A block is needed for overriding the parent html content, and other content related things. If the attach_library is indeed not working in an extended template, we may need to investigate why. That sounds like a bug.

Do you mean the extends need to be wrapped up in a {{...}} instead of {%...%}?

Sorry, I don't understand the problem yet, but if you think it is a bug would be great if you create a follow up explaining the problem and I will do my best trying to provide a fix/patch

Thanks!

Ack, ok. I see. attach_library is a function not a tag, so it can't be executed directly.

...I don't like the idea of modifying the parent template to make this work so it looks like #32 was actually correct.

In that case, we just need to fix 35.1 and this should be ready to go. :-D

Hi @gnuget,
Thanks for the explanation.

Updating the summary of the ticket and adding the novice tag.

Updating comment in patch #32

Hi @mohit_aghera.

Sorry the message is not enough, we need to explain WHY that CSS file is in the classy theme but is not used, I'm not a native speaker but we need something like:

This CSS file is not used in this theme (see https://www.drupal.org/node/2641380) but it's preserved so any other subtheme can use it. This file should be added in the image-widget.html.twig file.

Hi @gnuget
It's fine. I've updated message as per your suggestion. Please have a look at it.

If the attach_library is indeed not working in an extended template, we may need to investigate why. That sounds like a bug.

+1

I don't like the idea of modifying the parent template to make this work

+1

so it looks like #32 was actually correct.

-1

This is absolutely unacceptable.

This is a bug in Twig, because it definitely is intended to work this way — they even fixed a bug related to this some time ago: https://github.com/twigphp/Twig/issues/426. This is why I'm so confused about this not working. It really should work.

Actually, it works fine. See #2387069-47: {% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance + #2387069-48: {% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance, which also use this.

Actually, both of those are wrong: they're for set(), not for custom functions. There's several similar issues in Twig's issue queue: https://github.com/twigphp/Twig/search?q=A+template+that+extends+another....

I think the answer could be simpler in the mean time: let Seven's image-widget.html.twig do this:

{% include "@classy/content-edit/image-widget" %}
{{ attach_library('seven/image-widget') }}

Use #61 and #62 comment. Create patch.

Hi Chernous_dn

Here you are doing exactly the same as #32 (repeating a file in a different theme).

+++ b/core/themes/classy/templates/content-edit/image-widget.html.twig
similarity index 100%
copy from core/themes/classy/css/components/image-widget.css

copy from core/themes/classy/css/components/image-widget.css
copy to core/themes/seven/css/components/image-widget.css

I just attached a new version of the patch fixing this problem.

Looks good now, with the exception of the comment IMO still being confusing:

+++ b/core/themes/classy/css/components/image-widget.css
@@ -1,6 +1,12 @@
+ * This CSS file is not used in this (Classy) theme. But it’s preserved so any
+ * other subtheme can use it. This file should be added in the
+ * image-widget.html.twig file.

Here's my suggestion:

This CSS file is not used in this theme (Classy). It was intended to be used, but
due to a bug, Drupal 8 shipped with it not being used. To not break backwards
compatibility, we continue to not load it it in Classy. Every subtheme of Classy
is encouraged to use it, by attaching the classy/image-widget asset library in
their image-widget.html.twig file.

@see core/themes/seven/templates/content-edit/image-widget.html.twig.

@todo In Drupal 9, let core/themes/classy/templates/content-edit/image-widget.html.twig attach the classy/image-widget asset library.

I just updated the comment with your suggestion.

Thanks!

Thanks! I think it's ready now. Cottser should be the one to commit this. Also curious to hear what @davidhernandez thinks.

This looks much better than the last time I saw it, interesting use of include as well - I'm assuming someone has tested that :)

@davidhernandez what do you think about this approach?

NR for @davidhernandez' feedback, because I almost compulsively assigned this to @Cottser which would not have helped. :)

Are you all totally crazy? What a waste of time. I think we have more importants issues to fix and my first patch should simply get committed.

It was committed but you complained about it and was reverted.

But that let us work in a better fix so thank you :-)

@hass, feel free to spend your time on something else you feel is less of a waste. Participation here is not compulsory. There is no reason to be riled by a bug lkie this. It is easily fixable on a project.

Looking now.

1) What we have lost with this bug is the floating of the thumbnail, compacting that area. So it's good to keep this CSS and encourage its use, but it is not functionally critical. It's good to fix that in Seven.

2) The comment is fine. I didn't poor over it, but everyone else seems happy with it.

3) Classy is frozen, but this is fine to remove since it does not function and it does not alter markup.

+++ b/core/themes/classy/templates/content-edit/image-widget.html.twig
@@ -10,7 +10,6 @@
-{{ attach_library('classy/image-widget') }}

4) The alignment of the image and form needs fixing. I'm not sure if I would recommend doing that in the Classy CSS or just Seven, but that can be handled in a follow up issue. It is not the result of this issue, but this bug is probably why it wasn't noticed before release.

5) I hadn't noticed this before, but is there a reason why this was put into a subfolder in Seven? Seven doesn't use subfolders for any other templates. If there is no reason for it, this file should be in the main folder.

+++ b/core/themes/classy/css/components/image-widget.css
@@ -1,6 +1,16 @@
+ * @see core/themes/seven/templates/content-edit/image-widget.html.twig.

6) Classy should be capitalized in this comment.

+++ b/core/themes/seven/templates/content-edit/image-widget.html.twig
@@ -0,0 +1,12 @@
+ * Include from classy theme.

7) Not the fault of this issue, but we ended up with a blank line at the the top of the file, no @file comment, and no blank line before the rules begin. I'm not clear if we required @file in CSS as a rule, but this is consistent in all the other Classy CSS files. We should fix that while we're here.

+++ b/core/themes/classy/css/components/image-widget.css
@@ -1,6 +1,16 @@
 
 /**
  * Image upload widget.
+ *

Functionally, everything looks fine. The solution works. 5, 6, and 7 are my only issues. They are simple corrections. If those are resolved/clarified, the solution is RTBC to me.

Hi! davidhernandez

Here a patch with 5,6 and 7 fixed.

Thanks!

+++ b/core/themes/classy/css/components/image-widget.css
@@ -1,6 +1,16 @@
+ * @see core/themes/seven/templates/content-edit/image-widget.html.twig.
+ *
+ * @todo In Drupal 9, let core/themes/classy/templates/content-edit/image-widget.html.twig attach the classy/image-widget asset library.
  */
 .image-preview {
   float: left; /* LTR */

I hate being pedantic, but can you add the blank line after the comment to be consistent with the other CSS files?

Did you mean wrap to 80 characters or am I missing a standard that you mentioned in #81 @davidhernandez?

No, I meant the blank line after the comment and before the first set of rules. I don't know if we standardized it, but Classy's other CSS files are consistent with that.

Added the blank line between comments and first code line in all files needed

Taking also into account the 80 chars limit

Ok, removed the break line from twig files (my bad) and made the comment more readable (thanks @davidhernandez)
Sorry for the noise :/

Thanks!

Back to @Cottser then. Thanks all.

Looks good, thanks for the additional tweaks @darketaine @joelpittet @gnuget!

I made one minor change on commit and that was to move the library declaration higher up to preserve our beautifully alphabetized classy.libraries.yml, hope nobody objects. Interdiff attached to show that change.

Committed ae16101 and pushed to 8.3.x. Thanks!

preserve our beautifully alphabetized classy.libraries.yml

Nice attention to detail! :D

Can't believe I missed that. :( Thanks, Cottser!

Did that followup issue mentioned in 4) in #79 ever get created? We just have a customer reporting the suddenly floating image as a bug and it took quite a while to track the cause back to this issue, would probably have gone a bit faster if it had been tracked by an issue.

(I'll happily create the issue, just no point in make a duplicate if it has already been done)

(I'll happily create the issue, just no point in make a duplicate if it has already been done)

As far as I know, the follow-up was never created. please feel free to do it.

Thanks!