🇫🇷France @GaëlG

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

I just checked with the steps to reproduce and it's not reproductible anymore :)

gaëlg → created an issue.

Actually 🐛 Potential XSS vulnerability in colorbox and lazy load formatters Active is unrelated to this issue.

And 🐛 Broken thumbnail access when the default download method is set to "Private local files served by Drupal." Active is only necessary for this issue to work when default download method is set to private, but it's a separate bug which can be handled separately.

The only thing is making sure the latest one to be merged still works when the previous one has been merged, but it's the case for any issue.

OK, I updated the issue summary to show what's still needed.

FieldOutputTest seems to need an update to reflect the new way it works.

This seems to work. Please check it works the same as before, I'm not very used to these lazyload and modal features.

abhishek@kumar: no offense, your comment seems spammy/generated by an LLM. Please avoid adding verbose comments which do not add value to the existing issue.

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

Yes, the alternative would be that thumbnails remain always public, not private, like it was before. But in that case 🐛 Image style derivatives not being created due to schema mismatch Active should be reverted and reopen to find another way to fix it.

Still, I guess it might be better to allow private thumbnails (which is what 🐛 Image style derivatives not being created due to schema mismatch Active tried to do), so that thumbnails of private/confidential videos do not appear publicly on the web to people unauthorized.

I made a fix which seems to work well, but I'm not sure of the best way to inject the services (see @todo). And tests might be needed?

Thank you for your quick answer. Private files are only visible if some module (core or contrib) grants access to it. Usually, access to an image is given if the user has access to the content referencing the image.
So that if Video Embed Field uses private thumbnails, it must make sure the thumbnail access is granted if the user can view the entity which uses that thumbnail (this should be done with hook_file_download).

Actually I just discovered I had no problem with 2.5 because the thumbnail was always public before this change: https://git.drupalcode.org/project/video_embed_field/-/commit/acab726124...

After this commit, I suspect any website using private as default download method will have broken thumbnails (at least for newly embedded videos).

I'll try to propose a fix.

🐛 Potential XSS vulnerability in colorbox and lazy load formatters Active and 🐛 Broken thumbnail access when the default download method is set to "Private local files served by Drupal." Active have to be handled before this can land.

gaëlg → created an issue.

gaëlg → changed the visibility of the branch 3311063-add-support-for to hidden.

gaëlg → changed the visibility of the branch 3.0.x to hidden.

gaëlg → changed the visibility of the branch 8.x-2.x to hidden.

gaëlg → changed the visibility of the branch 3311063-add-support-for-cke5 to hidden.

I did not check the MR code but it seems to work. Thank you!

I rebased on 2.0.x and tests are fixed.

Hi there, I just made a module with this feature for Layout Paragraphs :)

https://www.drupal.org/project/layout_paragraphs_split →

Added a sheet with a comparison we made at Insite to choose the right page builder for a project we had.

baikho → credited gaëlg → .

Thanks for the review, I updated the IS.

Fixed by 🐛 Prevent saving config entities when configuration overrides are applied Needs work

gaëlg → created an issue.

In case it can help, I had a similar problem because the private image access system denied access to the thumbnail (403) because the image was not used in any viewable entity for my user (until I gave the "view config page entity" permission).

I also had another problem because I used the Webp contrib module before, which had created .jpg.webp files in the uploaded images private directory. Then I uninstalled Webp to use the core convert system. With this new system, it's not expected to have converted .jpg.webp files in the uploaded images private directory. These files should only exist in the styled (derivative auto-generated) images private directory.

Thank you for the work @vidorado! I cannot resolve threads either. @Matthijs?

@chizh273 If you click on the green button "Get push access" above, you should be able to update the MR. Anyway, @vidorado dit it.

@sourav_paul: I could find no easy way to reproduce precisely this on a clean install (such as https://simplytest.me) because there must be other render elements in pages also depending on url cache context, not just this one. Thus I get a x-drupal-cache:miss HTTP header as soon as I add a new query arg (such as ?foo), even without using "Taxonomy term ID from URL" default views argument.

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