🇫🇷France @GaëlG

GaëlG → made their first commit to this issue’s fork.

GaëlG → created an issue.

GaëlG → created an issue.

I just read about the Starshot project. This issue could be related?

Hi there, I just had to make some sort of review of the advantages and drawbacks of modern Drupal for a client's project. And I could not find up-to-date info about Drupal 8+ usage in the media sector (online press,...) : marketshare evolution, examples of famous big media websites using Drupal 8+ around the world,...
Another questioning I had is: are some of them still using Drupal 7, and if so, are they planning to move to another platform or to Drupal 10? Of course, the answer would be interesting but is hard to know.

Actually it's much more complicated, because if a facet is not empty on initial page load, but empty after AJAX reload (because another facet has been used), the empty facet will still appear (it's title at least). The whole facets block should be recomputed on AJAX reload, but it's not the case.
Actually, I'm not sure this module is so useful, as blockgroup can do kinda the same.

I first updated MR !2, then I found out that MR !2 was (wrongly it seems) against 1.x. So I made MR !12 with the same changes but against 2.0.x

I switch the issue status to postponed as 🐛 Facets with AJAX not working in most of situations RTBC is not committed for now.

GaëlG → made their first commit to this issue’s fork.

Actually weight is discouraged ( https://www.drupal.org/docs/develop/theming-drupal/adding-assets-css-js-... → ) and I now think the right fix is to load only one language file (no need for .en.js if .fr.js is loaded).
So I updated my merge request. It seems to work well.

I don't think so, as .en.js just defines the tarteaucitron.lang variable and .fr.js just redefines it.

Actually my second idea seemed easier to do: if a config exists by default (in sync) but not when the split is active, we config-split:export that config as an empty array yaml file. Then on config import, we delete this config if it exists, in case we detect an empty array.

I made some manual testing, for my use case but also for a "normal" config entity (not a translation one). It seems to work well. But I would not be surprised if my code has problems I did not expect. This code seems complex to me and I think it's the first time I contribute to it. I was not always sure of the meaning of every line, some where sort-of copy-pasted.

Anyway, I made a merge request. But even if the idea is OK and the implementation is OK too, I guess tests would be needed?

The good news is that this would give a new feature: remove a config only in a split.

Yes, exactly, that's one way to fix that I have in mind. I first used the weight system instead because technically, from an point of view agnostic from context, it's not required that .en.js is loaded for .fr.js to work. It's just that if .en.js is loaded, it has to be loaded before .fr.js.

I mean:

#1 .fr.js
#2 .en.js
KO

#1 .en.js
#2 .fr.js
OK

#1 .en.js
OK

#1 .fr.js
OK

If the two ways (dependency or weight) actually work (I'll check that), I'll ask you which one you prefer.

I tried to reproduce with https://simplytest.me/ (tacjs 6.5 on core 10.2.5):
- enabled Interface Translation
- added French language
- saved TacJS settings with "Default state: Wait"
- checked "Need consent" for FERank service
- cleared the cache
- went to french home (/fr) in a new private navigation
Result: correct translation

- disabled JS aggregation
- cleared the cache again
Result: still correct translation

tarteaucitron.fr.js is always loaded after tarteaucitron.en.js, which is right.

Still, on my project, I get the reverse order so that translation doesn't work. I found out that this happens when I attach a custom lib in a template (with Twig function attach_library()) which declares its dependency on tacjs/tacjs (because it adds an entry to tarteaucitron.services). As soon as I remove the dependency, it works again.

So: is it a bug in my custom code or a bug in the module?

If I check the code, I can see that:
* tacjs/tarteaucitron.en.js is a dependency for tacjs/tacjs which is attached in tacjs_page_attachments().
* tacjs/tarteaucitron.fr.js (declared in tacjs_library_info_build()) is also attached in tacjs_page_attachments(), afterwards. It depends on tacjs/tarteaucitron.js but not on tacjs/tarteaucitron.en.js.
And none of the two libraries have a weight. Thus, with no weight nor dependency, loading order is not guaranteed. It happens that on simplytest.me the order is OK. But it's not on my project.

To me, tacjs module should make sure tacjs/tarteaucitron.fr.js is always loaded after tacjs/tarteaucitron.en.js

Anyway, I set to "needs work" for now (assigned to me), because it looks like my merge request causes another bug. I have to check that.

It should be better now, feel free to suggest improvements.

1.x and 2.x are unsupported, so I guess it's outdated. Feel free to reopen if needed.

GaëlG → made their first commit to this issue’s fork.

OK, then I'll investigate further. Thank you for your quick answers!

GaëlG → created an issue.

We use it on many websites too, and never had the problem before. But now we use as a patch this commit from 🐛 Tarteaucitron translation files can be loaded before the main tarteaucitron js Fixed (which is merged in dev but is not included in latest sable 6.4.0): https://git.drupalcode.org/project/tacjs/-/commit/a8e29ca21daff3a26cbe8f...

I guess it made the bug appear.

GaëlG → created an issue.

Thank you @mgifford. I can see that in Wagtail, they seem to have no default value for the sizes and srcset values. So it's up to the "site builder" to define the right values, which can be difficult. I guess in Drupal we should have a decent default, if possible.

https://docs.wagtail.org/en/stable/reference/jinja2.html#picture

GaëlG → created an issue.

I made a fix so that browser URL is not updated if the view is in a dialog/modal, because in that case the browser URL is not the URL containing the view parameters.
It's like there's an URL for the page which is behind the modal (the one shown in the browser URL field), and one other URL ("hidden") for the "page" shown in the modal.
Without this fix, we got a strange behavior when using media library search in modal: it works the first time, but as soon as params are added to the browser URL, subsequent search is buggy until the whole page is reloaded.
I guess a test should be added for the modal case? Like just test that browser URL is not updated when an AJAX exposed form is used inside a modal.

And automated tests do not pass, I guess it's just some test code that needs to be updated for compatibility with current 11.x.

Then, I believe the merge request will be ready for review again.

GaëlG → made their first commit to this issue’s fork.

GaëlG → made their first commit to this issue’s fork.

The change proposed in #2 still is in the Drupal 7 version of the module. I don't get why it shouldn't be in the D8+ one. The problem encountered in #3 may have another cause.

Note that there is a contrib module which provides this kind of feature, even for CKE5. In case it can help.
https://www.drupal.org/project/inline_responsive_images →

I needed this on D10 but it was reverted for no given reason. So I made this branch, still untested: https://git.drupalcode.org/issue/oauth2_server-3288840/-/tree/3288840_29...

GaëlG → made their first commit to this issue’s fork.

This may be related to 🐛 500 error with JSONAPI on Base Entity Fields Needs review .

This bug cannot happen anymore since 📌 Cleanup Drupal 8 code paths Fixed is done (>=3.24-beta1).

@hmendes: Yes, I think so.

Hmmm... actually the bug I get is not quite the same as the one from this issue. I cannot reproduce the exact bug described in this issue with 4.0.0-alpha5, even without a patch.

(and the fix I made in the merge request does not fix my bug, I'll open a new issue if needed)

Just rebasing does not work. The code in case ConditionalFieldsInterface::CONDITIONAL_FIELDS_DEPENDENCY_VALUES_WIDGET changed, so I guess we must take the new logic.

I just rebased against 4.x

GaëlG → made their first commit to this issue’s fork.

With CKEditor 5, included in Drupal 10 core, it looks like media caption is already not editable. CKE4 is unsupported and needs a contrib module.

Can this bug be reproduced on a clean Drupal 10 install, i.e. with CKEditor 5? I don't think so. At least on my test instance I'm able to change the alt and alignment of the embedded media thanks to the new CKE5 UI.

As CKEditor 4 is now contrib and unsupported, I guess this issue should be in CK4 contrib and not in core?

The patch does not apply anymore, and the Drupal core issue is merged.

@daften. Thank you! It should make it easier for the maintainers to merge the request, when the issue is reviewed. The thread creator may be the only person allowed to close a thread? Nevermind.

I just made a commit which fix the problem on my test website. The current version of https://git.drupalcode.org/project/editor_file/-/merge_requests/4.diff is testable.

@daften: Could you please marked the resolved threads as resolved on https://git.drupalcode.org/project/editor_file/-/merge_requests/4#note_1...? And if they are all resolved (right now it looks like they are), mark the merge request as ready?

From manual testing, I can confirm that, right now, the patch being built in merge request #4 (current state at https://git.drupalcode.org/project/editor_file/-/merge_requests/4.diff) works when the Linkit plugin is enabled, and doesn't work otherwise (TypeError: this.linkUI.actionsView is null).

I just released https://www.drupal.org/project/ckeditor_small_tag → for this use case.

@flyke @claudiu.cristea @Martijn de Wit @BramDriesen: please see #447 and #451.

I did not test it yet, but this module seems to address that need for CKE5: https://www.drupal.org/project/ckeditor_link_styles →

See https://www.drupal.org/project/drupal/issues/3030640 📌 [policy, documentation] Clarify Constructor BC policy and document best practices for subclassing other classes Active for why the same doesn't apply to core.

@akalam I ended here searching for the same. After a bit of reading, here's what I found out:

• AFAIK, there is no official way to subclass dependency-injecting classes (services, plugins,...).




• For services, it's usually better to decorate than subclass when possible (for plugins, I guess it's too complex to decorate).




• It's not official, but if you have to subclass and do not control the parent class (you're in custom or contrib and parent class is in core or other contrib), you should not override the constructor but rather inject additional needed services directly in create(), as explained in various blog posts such as https://www.hashbangcode.com/article/drupal-9-extending-drupal-base-clas... and https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...




• Why? Because your code won't break if the parent constructor changes its signature.




• Drupal core still overrides constructors because it can: it controls the parent classes, which are in core too. This may explain why the bad practice of overriding constructors is widespread, because core is taken as an example.




• Solely injecting additional needed services in create() and not in constructor can be a problem in one case: unit tests. So, if/when you need to unit-test your class, you can declare a setter that allows you to set the service just after construction.

Just referencing this blog post which may help with step 1 of comment #22: https://mariohernandez.io/blog/responsive-images-in-drupal-a-guide/

I also talked to @nod_ which is Provisional Frontend Framework Manager and may help take a decision on step 1.

I tested and confirm that the merge request successfully adds width and height HTML attributes to img tags with an SVG rendered using the default image formatter with an image style applied.

Yes, deprecation should rather be discussed in 📌 Deprecate not giving cacheability metadata to forms Needs work . I answered there.

The conversation about deprecation started in 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review , but I continue it here as it's now out of the scope of the former.

@catch #2578855#138: Yes. The question is: is it OK that some forms trigger no deprecation? They can have bad cacheability metadata, which until now leads to no bug, but will lead to bugs when we will ultimately default to cacheable.
And if the answer to that question is no, then another question arises: how to make sure every form maintainer and form_alter maintainer (has pointed by @jonathanshaw) ensures it gives the right metadata?

Note that $render_array ['#cache']['max-age'] is sometimes set "by default" in a cache metadata handling method, and not explicitly by the developer (such as by addCacheableDependency() if I remember well). This explains why I had to force opt-out some forms in my merge request, in EntityFormDisplay, ContactController and NodeForm.
I'm not sure it invalidates your proposals, but I just wanted to make sure you took that into account.

This issue could interfere with 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review if it's done before it. But it's not too soon to think about how to implement the deprecation.

I still have to fill the descriptions of the 3 new issues I reference in code changes, but otherwise it's ready to be reviewed :)

PS: Some related bugs and tasks have been discovered during the course of this issue, so some other d.o. issues still should be created for them. See #119.

Just changed the issue title to better reflect what's in its scope.

GaëlG → created an issue.

GaëlG → created an issue.

#126: Oh that's a good news! Anyway, as said in #123, I guess this CacheableDependencyInterface thing can be done in another issue: https://www.drupal.org/project/drupal/issues/3395157 📌 Deprecate not giving cacheability metadata to forms Needs work

#127: Thank you, this may indeed be a good way to do! For now I'm still trying to work the other way around, so that cacheability on some forms can be made available sooner, even if some other forms still have bugs.
So, I just set the opt-in system (https://git.drupalcode.org/project/drupal/-/merge_requests/5047/diffs?co...). Theoretically, it should change nothing as no form should already use an opt-in we just "invented". But in practice, tests fail because some forms still already opt in by accident because of the problem mentioned in #74. For them, I'm willing to apply the method you describe in #101:

The entity form display cache data is currently unused, all we need to do is add a temporary explicit max-age=0 to it with a @todo to remove that in a follow-up issue. This is less problematic than YetAnotherMysteriousRenderArrayKey.

GaëlG → created an issue.

Actually, I attended to @xjm's DrupalCon Lille session about core issue reviews yesterday (https://t.co/Z5YLHtZwqr), and I'm now approaching this issue with a new look.
It started 8 years ago, has 62 followers and still, it seems it's not even close to be committed. I guess it's a good example of longstanding issue because of "scope creep", as already mentioned in #100. We need to break this issue into small pieces.

So, even if we all seem to like the CacheableDependencyInterface idea from #110 (me too), it may not be a good idea to do this within the scope of this issue. After all, we already have something to give cacheability metadata: the #cache which comes with any render array (and FormInterface::buildForm returns a render array). So, like for blocks, using CacheableDependencyInterface would allow developers to use a more OOP way to give cacheability metadata, but they could still use #cache directly. So let's stick to #cache for now, and the CacheableDependencyInterface can be a follow-up.

At least that's my view and the way I'll try to work on the issue (yes, I also saw the Sarah Furness keynote about not having fear of making mistakes 😀).

The very first thing we need, and probably the only one that should be done within this issue, is: allow forms to opt-in to be cacheable. The simplest way possible.

I was there

I was there :)

I was there!

I was there :)

I guess we cannot just make FormInterface implement CacheableDependencyInterface because it would break any existing form directly implementing FormInterface without extending FormBase.
So... we could create something like CacheableFormInterface (it is the right name, as actually it could be used for an uncacheable form?) which would of course implement both CacheableDependencyInterface and FormInterface.
Then, where the buildForm() method is "consumed" (I got no time to figure out where it is for now), check if the form implements CacheableFormInterface and if so, add the #cache to the renderable array.
And progressively, make classes implementing FormInterface implement CacheableFormInterface.

We use and re-roll this useful patch for ages, it may be the right time to stick my head in it and try to move it forward!

What the current patch contains:

1. A removal of the bad-for-performance default cache max-age set to zero in FormBuilder.php. With only this, bugs happen because some forms do not declare well their cacheability metadata.
2. A system based on the new opt-in form key #form_cacheable which:
   • - makes forms still uncacheable (to prevent bugs), but:
     - only if #form_cacheable is not explicitly set to TRUE
     - and by acting on subform $form['form_token'], not on "root" $form (because it makes it clearer that it's the token which prevents the form from being cacheable)
   • - changes some core forms so that they become cacheable, by adding the correct cacheability metadata - and tell they now are, with #form_cacheable:
     - CommentForm (with an update to the corresponding test: CommentDefaultFormatterCacheTagsTest.php)
     - MessageForm (+ContactSitewideTest.php)
     - NodeForm
   • - adapt a unit test for that: FormBuilderTest.php
3. Correct cacheability metadata on some other form elements (+test changes) :
   - core/modules/editor/src/Element.php (+ EditorLoadingTest.php + EditorSecurityTest.php)
   - core/modules/filter/src/Element/TextFormat.php
   - NodeSearch.php (+SearchAdvancedSearchFormTest.php )
4. Some code to ensure node forms remain uncacheable (NodePreviewController.php, NodeForm.php) (comment #17, but I guess it's now useless as form cacheability has been switched from default to optin during discussions)
5. A bug fix (+test coverage in field_test_boolean_access_denied.module, BooleanFieldTest.php, FormTest.php ) for the fact that form widgets #access default value is sometimes a boolean whereas it should be an AccessResultInterface object. See comment #15.
6. A work-around introduced in #15 to get rid of a problem at rendering (in Renderer.php, RendererTest.php )
7. A bug fix and tests related to language (see comment #15) : language.module, LanguageConfiguration.php, ContentLanguageSettings.php, LanguageConfigurationElementTest.php, NodeTypeInitialLanguageTest.php, EntityTranslationFormTest.php
8. Something to get rid of a bubbling error (#118)

What needs to be changed in the patch (probably rather by creating an issue branch from scratch), according to discussions above (#100, #101, #110, #116 mainly):

1. Update the issue summary with part of this comment
2. Do not keep the #form_cacheable system
3. Do not keep the various cacheability metadata fixes, nor their unit tests (all of these will be handled in follow-ups, see #100)
4. Initiate those follow-ups
5. Do not keep the probably useless changes in NodePreviewController.php and NodeForm.php
6. Move the #access bug fix to a new related issue
7. Do not keep the Renderer.php/RendererTest.php workarounds (not sure about this one, but I guess it's needed only for some follow-up)
8. Same for the language bug fix
9. Same for the bubbling error #118
10. Make forms implement CacheableDependencyInterface so that they provide cacheability metadata
11. in FormBase, provide the default implementation of the interface, which set the max-age to zero as soon as there's a form token
12. If there's still a problem with entity forms (see #74 and #101), "fix" it by explicitly marking them uncacheable + creating a @todo follow-up to make them cacheable correctly
13. Deprecate this base implementation (so that developers are encouraged to implement it with correct metadata)
14. Create issues for future major versions of core, according to #101 : 3. and 4.

@vsujeetkumar: Thank you for your work, but as said above, please put this patch into #3392556 contrib issue instead. There is no chance for it to be accepted for core as it targets CKE4.

Please put CKE4 patches in the newly created ✨ Allow image style to be selected in Text Editor's image dialog Active . Patches here should contain a CKE5 plugin.

GaëlG → created an issue.

I just published the dev package

Yep, as far as I know, it's discussed in comments #423 to #425 above but has not yet been addressed.

The current patches are for CKE4 and should rather be in a CK4 contrib module issue, probably depending on this one or restricted to Drupal 8/9. And this issue should be focused on core/CKE5, in which case it should be considered as having no patch for now (I just unchecked "display" on the available patches).

If you need this feature on CKE5, the best available option for now is the Inline responsive images contrib module with a patch from 📌 Compatibility with CKEditor5 (default in D10 core) Needs work .

Note that Webform Entity Print is not required if you simply want to get a PDF version of a submission. Entity Print seems to be enough. Webform Entity Print adds a download link, admin UI, integration with webform exporters,...

Yes. On my side I use the Linkit module, it may explain why it works for me. Anyway, there's obviously something to fix.

Yes. On my side I use the Linkit module, it may explain why it works for me. Anyway, there's obviously something to fix.

Do you have the link plugin enabled in you CKE format? I'm wondering if we may miss something in the config form to require it. Not much time right now to investigate, but I guess we currently cannot have a toolbar with a "add file" button but no "add link" button.

@percoction: I guess this bug will not happen anymore as:
- LinkUI is now required: https://git.drupalcode.org/project/editor_file/-/merge_requests/4/diffs?...
- we use LinkUI after it's been initialized: https://git.drupalcode.org/project/editor_file/-/merge_requests/4/diffs?...

@catch #23: Sorry, I wasn't clear enough. Yes, picture/source can be useful because they allow for file type fallbacks, media queries (notably for art direction),... And even if a picture tag with only one child source tag can be simplified into an img tag, it's not necessary.

I meant: what is a bit more controversial is: most of the time, to display an image (usually a photo), should we use:
- sizes?
- and/or media queries?
- and/or multipliers?
If we use srcset, which widths should be provided? Should we have a different image quality (I mean compression in %, not size in px) depending on the multiplier?

In other words: if I have a source image file and a desired aspect ratio, what should be the resulting HTML, by default in core? My current answer is in the issue summary, but I'm not sure it's the right one. I did not know about sizes=auto but it looks great, as it prevents the need to set the right sizes value according to CSS, which can be difficult and can't be computed as a default.

@heddn #24: Yes, the big need is point 2. But it's easier to discuss if we agree on point 1 before.

@seanB #25: This is now a meta issue :) But before creating most children issues for Drupal implementation tasks (I like your ideas), I think we need to agree on point 1.

@prudloff #26: Yup, there are two ways to serve image formats with fallbacks. One is to look at HTTP_ACCEPT header and serve a different HTML according to it. The other is to use a picture tag. The second one seems better to me because it eases caching. But this is one of the questions on which we should agree before we can set a default system.

Hello there, thank you all for the great feedback on this issue! It looks like many people are concerned and have ideas! :)

It may be the good time to start organizing a bit the various problems and ideas discussed here. It's look like there are 3 main questions.

1) What should be the decent/recommended/optimized/default way to display an image in HTML?

picture, source, srcset, sizes, image file formats, multipliers, image quality, breakpoints,... This issue in somewhat Drupal-agnostic, I guess we can get answers elsewhere on the web.

Avenues in comments #1, #4, #17, #20

See also:
https://www.stefanjudis.com/notes/should-responsive-images-work-with-con...
https://github.com/whatwg/html/pull/8008
https://ausi.github.io/respimagelint/docs.html

2) Which technical way to share/pool the creation and also the updates of many Drupal image derivatives needed for a same responsive image display, to get a better admin UX?

ImageStyleInterface should not extend ConfigEntityInterface? Or we should use config at the field formatter level and get rid of the Drupal "image style" concept?
Wizard à la Views?

I believe we need a clearer view in question 1 before going further with this.

Avenues in comments #1, #16, #17, #18, #19, #21

See also:
https://www.drupal.org/project/easy_responsive_images →
https://www.drupal.org/project/responsive_image_automatic →
https://www.drupal.org/project/drimage →
https://www.drupal.org/project/image_styles_builder →
https://www.drupal.org/project/image_style_generate →
✨ Responsive image format for media Needs work
🌱 [META] Usability of Responsive Image in Core Active

3) Webp and avif conversions by default in core, with fallbacks (at least for avif which is less supported for now)

This seems to be more a task than a question, as there is some consensus on what should be done (maybe not that much on how in should be done?). This is already a work in progress independently from this issue. There could be a follow-up to question 2 to handle question 3 in the new UX paradigm.

See also:
https://www.drupal.org/node/3171135 →
✨ Add fallback format support to responsive images Needs work
📌 Add webp image conversion to core's install profile's image style Fixed
✨ Allow user-uploaded WebP images everywhere in Drupal by default: image fields, media library, text editors Needs work
✨ Let GDToolkit support AVIF image format Needs work
https://www.drupal.org/project/wpf →
and comment #9

So, do we make this issue a meta and create 3 children issues?

Thanks! :)