🇫🇷France @GaëlG

gaëlg → created an issue.

Hello bircher, I made tests and added some comments to the MR.

prudloff → credited gaëlg → .

📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review is fixed :)

MR40 looks good to me. If it works, it should be committed.

@mirakolous The patch generated from the MR (https://git.drupalcode.org/project/svg_image/-/merge_requests/18.diff) applied on 3.0.1 some days ago.

I made a new branch which also contains the fix from 📌 Formatters shouldn't repeat core code Needs review , because I needed both fixes:
https://git.drupalcode.org/issue/svg_image-3126330/-/commits/3257729-312...

The code for this issue changes quite a lot when 📌 Formatters shouldn't repeat core code Needs review is applied.

On this branch, I also added support for responsive images. I guess my branch could become the one for an up-to-date merge request if 📌 Formatters shouldn't repeat core code Needs review lands before this one.

I manually tested locally and it seems to work well on my use case but I did not carefully tested all the possible usages.

Actually, the current MR from 📌 Formatters shouldn't repeat core code Needs review is enough to fix this problem. Should we mark this a duplicate?

This code is executed twice because it looks like we need $images in the overriding method, to loop on it. It seems there's no clean way to do otherwise without re-implementing the parent method. Ideally, the parent method might be split into several ones so that we don't have to make that second call, but from a contrib module point of view, we can't change core code.

Yes, this is not perfect because it could have a negative impact on performance.

BUT performance is not the only criteria. I this case, I suspect the second call will be very fast and lead to no additional SQL query thanks to various Drupal cache layers. Additionally, the result of this execution can also be cached (render caching).
So I'd say we can trade a tiny performance degradation to get a contrib module that has much more chance to work well with subsequent core versions.

For anyone facing the same: I had a problem with Devel module when dumping objects or arrays through dpm(). Everything normally collapsed (sub-arrays,...) was expanded and not collapsible.
I could get rid of the problem by disabling Big Pipe (and Big Pipe Sessionless). Or by applying patch #45.

I worked on this one year ago.
I just read the change record and checked https://www.drupal.org/community/contributor-guide/task/write-a-change-r... → .
It looks fine. There's only the "Introduced in version" field that needs to be set to the right value.

I do not have all the specific elements in mind, but generally, yes, I'm in favor of splitting this kind of big old issue in several smaller ones, so that they can be reviewed more easily. That's the kind of practice encouraged by core reviewers: https://www.youtube.com/watch?v=_uCfmFH4rUs

gaëlg → made their first commit to this issue’s fork.

gaëlg → changed the visibility of the branch 3275497-add-arity-key to hidden.

gaëlg → made their first commit to this issue’s fork.

I rebased the ...-10.1 branch against core 10.3.x, so that I could generate a patch against 10.3.5.
Not sure I did the right changes on core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php, I just blindly made the same number changes (if the commit added 42 to some number, I added 42 to that same number from core).

gaëlg → made their first commit to this issue’s fork.

I just released RC1

gaëlg → made their first commit to this issue’s fork.

Whoops, I merged but forgot to update this issue's status.

I think you should have a "get push access" green button above, to alter the MR.

MR !19 fixed the bug in my case, thank you gslexie!

I don't change issue status as the review should probably be more than that.

Also, it seems it's more a bug than a feature. Pages get moved in the menu without anyone wanting that.

GaëlG → created an issue.

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 Needs review 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.