🇮🇹Italy @plach

I had a look at the MR and did some testing and this looks good to me as well, thanks!

amateescu → credited plach → .

amateescu → credited plach → .

@lauriii

However, many of these sites are re-using media and it would be great if we could solve this in a way that doesn't get rid of the re-use media part. I think there are ways in which this could be solve so that we would have both.

In terms of what is possible right now, the widgets defined by Media Widgets were thought explicitly with non-reusability in mind: they mimic the Media Library widgets without offering the ability to select existing media. A site needing to support use cases could certainly enable both the Media Library widget on re-usable media reference fields (e.g. an article content type only authored by the site's editorial team) and the non-reusable Media Widgets for other use cases, e.g. user pictures.

An approach I've been thinking about, but that I didn't have the opportunity to explore is conditionally enabling the existing media selector depending on permissions: a less privileged user could only upload new media items (or enter new URLs), while a more privileged one could have access to the Media Library as well. From the implementation perspective, this would likely mean always using the Media Library widget and tweak its logic to make access to the existing media selection UI conditional.

While 📌 OOP hooks using event dispatcher Needs review is a huge improvement (and one I'm deeply grateful for :), I think it's undeniable that having both Symfony Events and Drupal Hooks at the same time means having two competing implementations of the observer pattern. In most cases there is not a clear reason to pick one or the other, if not a matter of "taste". In an ideal world we'd have a single system covering all use cases and core would promote consistency in contrib by only using that. In an ideal world both Symfony and Drupal would use that same system :)

Here is the corresponding core issue (postponed on this one):

🌱 [PP-1] [Meta] Consider adopting Media Widgets as an experimental feature Active

Details to be fleshed out, once experimentation has happened in 🌱 [Meta] Consider adopting Media Widgets as an experimental feature Active .

plach → created an issue.

More info is usually better :)

plach → created an issue.

plach → created an issue. See original summary → .

(Wrong issue, sorry for the noise :)

Looks good and works well, thanks!

Looks good to me but we need tests: converting #31 would be a good start :)

Looks good and works well, thanks!

@amateescu's review was addressed and this works fine here. I think we're good to go.

Sorry, minor cleanup.

This was not working as intended: I got errors when testing this with legacy redirects containing absolute URLs. The attached PR just turns the redirect into a relative URL.

Thanks!

This is looking good and working well, but I'd perform a small tweak.

Rerolled the patch on the current HEAD. Raising priority to major since the performance gain may be significant.

Vaguely related issue: 🐛 Compressed aggregates not delivered on first request Needs work .

plach → made their first commit to this issue’s fork.

That's what we are changing here, so far 5.0.x is just a copy of 4.2.x.

Sorry for the belated feedback :)

We have plenty of versions supporting all sorts of combinations of legacy versions of Drush and Drupal. I think for Drupal 11 it makes sense to drop support for the older stuff.

The solution to restore the primary key and default to 0 for the int ID makes sense to me: users are not supported and even if they were, the anonymous user wouldn't be editable. Any content entity out there with numeric ID relying on core base classes will start from 1, so I think this is a safe approach.

The MR code has been working nicely in production for a while now, except for the latest update, and tests look good, so I think we are done here.

4.2.x targets all versions of Drush >= 11

poker10 → credited plach → .

Looks good and works well here :)

If you are using PHPStorm, please upvote the related issue. This should increase the likelihood we get a fix in timely fashion.

I pushed a commit to take removed entity types into account, as I got the following error when trying to run the latest wse updates:

>  [notice] Update started: wse_menu_update_10002
>  [error]  The "wse_menu_tree" entity type does not exist. 
>  [error]  Update failed: wse_menu_update_10002 

Just for posterity reference: I asked @amateescu why we are not just converting the target_entity_id field type from int to string. The reason is that Postgres can't do joins on columns with different types.

Thanks for the offer: the 4.x version of the module was developed as part of the launch of a D9 website, which is actively using it, however at the moment there is no additional development foreseen. I'm open to grant co-maintainership to other people, my main concern is that the functionality that was built so far keeps working as originally designed, so that our project is not negatively impacted.

Given how common the image/media use case is, compare to text only, I think we could even have an media entity reference defined in code as a base field.

I agree this would be desirable, contributions welcome :)

Looks good and works well, thanks!

+++ b/modules/wse_preview/src/WsePreviewWorkspaceManager.php
@@ -0,0 +1,138 @@
+  public function __construct(WorkspaceManagerInterface $inner, CookieWorkspaceNegotiator $cookie_workspace_negotiator, RequestStack $request_stack) {

Nit: we can rely on constructor property promotion here :)

Committed and pushed to 4.2.x, thanks!

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -281,6 +281,16 @@ protected function load() {
       return FALSE;
...
+      $this->logger->error("The image toolkit '@toolkit' failed loading image '@image'. Failed function: @function.", [
+        '@toolkit' => $this->getPluginId(),
+        '@image' => $this->getSource(),
+        '@function' => $function,
+      ]);
+      $this->preLoadInfo = NULL;
+      return FALSE;

Instead of duplicating this logic, why don't we just set $image to FALSE in the catch branch?

catch → credited plach → .

@sathishkumar

Thanks for the patches, but the changes will be merged to the development branch first, then backported.

plach → created an issue.

Working on this.

plach → made their first commit to this issue’s fork.

FYI queue/async support for Symfony Mailer is being added at ✨ Add Symfony Messenger support for async messsages (emails as queues) Active .

Looks good and works well here, I pushed a small commit to address @graber's review.

I only changed a word in a comment, so I think it's fine for me to RTBC this :)

plach → made their first commit to this issue’s fork.

I had a look and I'm not sure that providing a ResourceTranslationTestBase class extending ResourceTestBase is the way to go: we would need to duplicate the children classes (e.g. EntityTestTest), reimplementing the abstract methods twice with the same logic.

I'd rather provide a test trait that could be used by both JsonApiTranslationFunctionalTest and any resource-specific test class, e.g. EntityTestTranslationTest extending EntityTestTest. We could then extend TestCoverageTest to check for the translation test classes presence and ensure all resources also provide translation-test coverage.

@Wim Leers

Thanks for the review and sorry for the belated reply!
(I'd need more birthdays to work on this regularly ;)

For now, I'm especially interested in understanding why this is not extending ResourceTestBase but JsonApiFunctionalTestBase.

I think (it's been a while for me as well), I was just hoping that as an experimental module we could get away with only generic test coverage, as providing test coverage for every entity type seemed overkill at the time, as we did not discuss any entity-type specific logic. OTOH since the scaffolding is already there now, I assume it should not be a huge amount of work to try and leverage ResourceTestBase instead. I'll give it a try, stay tuned :)

Hey, so the D9+ branch is a complete rewrite and is very limited in scope: basically only the features that were needed by the organization who sponsored the work were ported, so no scheduling is implemented at the moment. For the same reason, permissions are not very granular: anyone with the Administer ADs permission can create/edit any AD.

plach → changed the visibility of the branch 3199697-add-jsonapi-translation to hidden.

Ok, this should be ready for reviews for reals now :)

Looks good to me and works well here, thanks!

Bug fixes need to be applied to the dev branch first and then backported :)

It seems the use cases described in the OP could also be addressed by #1234624: Limit the number of fields to display → .

Looks good to me, I'll manually test this for a while and then report back.

@Graber

Could you please add some details to the IS Problems/Motivations section?

@Berdir

Note that the other issue also tried to handle skipped deprecations, which is IMHO crucial for this, as some of them have been logged a _lot_ historically and would completely spam that log, making it rather useless.

Fair, what about making those configurable via a setting as well? We could hardcode a default if the setting is NULL.

What about using PHP settings to make the behavior configurable? We could make this opt-in by assuming a FALSE default and punt aggregation/filtering to a follow-up issue :)

Isn't this a duplicate of 📌 Add option to log deprecation errors to prepare for Drupal 11 Needs work ?

Sorry, but this module is not meant to help during upgrades for the reasons explained at https://www.drupal.org/project/devel_entity_updates/issues/3082442 → .

I realized I didn't properly implement @UsingSession's suggestion. I did it now, hopefully :)

Looks good and works well, thanks!

The changes in the MR make things way cleaner, but they seem out of scope for this issue. I'm attaching a patch with the only change that I think is required to get rid of the warning. It would be good to create a separate issue to clean up service injection.

@mahyarsbt

Could you please publish your code as a sandbox, so we can evaluate your request properly?

@gabesullice

Addressed your reviews at https://www.drupal.org/project/drupal/issues/3199697#mr1591-note220760 📌 Add JSON:API Translation experimental module Needs work

The commit above adds GET test coverage, the rest is still to do.

@bbrala @bradjones1

I resumed work on this today at the DrupalCon Lille sprint. Regarding #143 - #144, the latest code uses a langCode parameter. That looks compliant, doesn't it?

I'll try to push this forward a bit :)

Looks good and works well, thanks!

Only a minor nit, looks good and works well, otherwise :)

+++ b/fivestar.tokens.inc
@@ -0,0 +1,72 @@
+    !\array_key_exists('fivestar-rating', $tokens) &&
+    !\array_key_exists('fivestar-count', $tokens)

The leading slash is redundant :)

!array_key_exists(

Removed unwanted newline

plach → created an issue.

@pobster

Shouldn't this be committed to the 5.x branch as well?

This is a reroll (+ typehint cleanup) of #3, which is working for our use cases (webp conversion).