🇮🇹Italy @plach

amateescu → credited plach → .

Test looks good!

Test looks good!

This is working well in our project, we "just" need to add test coverage now :)

This is working well in our project, we "just" need to add test coverage now :)

This is working well in a project of ours, tentatively marking RTBC :)

Looks good and works well, thanks!

Looks good and works well :)

plach → created an issue.

plach → created an issue.

amateescu → credited plach → .

So again, it's just confusing what site maintainers are supposed to do about these warnings

The short answer is “nothing”: those errors should be fixed via an update function by the module providing the field definitions. I’ll try to expand on this when I have a keyboard under my fingers 😉

Sometimes a change occurs in a field definition property not having a direct impact on the DB schema, e.g. the field label, but those count as mismatches as well.

quietone → credited plach → .

MR merged

@berdir

Do you think the solution proposed in #34 would be acceptable/viable for the use cases you summarized? I guess we could skip deprecating the global contextual storage, at least until we are done cleaning up all those occurrences.

You can kind of can register multiple services with a service provider, instead of the plugin definition you could pass in that logic in the constructor. [...] But you could also loop over something within the service itself.

I can imagine passing something analogous to a plugin definition stored in the .services.yml file via service arguments, but I'm not sure I follow your suggestions: would you mind posting some pseudo-code? :)

@berdir

That's the tricky part. There's a surprising amount of places where ->_referringItem is available that would possibly need to be refactored to be able to receive that. FWIW, most places probably _do_ have access to the render array, similar to your view example, so that might be a feasible option.

Right, we could definitely add the information to the render array instead of discarding it and deprecate accessing _referringItem. This should definitely be ok BC-wise.

That was my reason for going with the getOriginal() method

IMO this is completely fine: the original object is not random/contextual data, I cannot think of a more legitimate use of a property on an entity object :)
If we ever end up introducing the distinction between immutable and mutable entity object, then a ::getOriginal() method would only make sense on the latter (and return the former), similar to the ::isNew() one and friends, but for now I think that's a valid solution.

I guess my concern is that if we have to solve every single instance of these current cases, deal with BC and so on then we won't be able to do something about 📌 [meta] Deprecate __get/__set() on ContentEntityBase Needs work for many, many years.

Maybe we could use magic methods to populate a globally available EphemeralContext instance and stop storing them on the entity object? This way, in the BC phase we can throw deprecation messages warning people that they should not use magic methods and that they should refactor their code to pass data around properly, but at the same time we can drop magic methods and leave the global EphemeralContext instance around for (at least) one more major release, so that people not having the option to refactor their code can access the global context directly instead of via $entity->__get()?

Pseudo-code ("tested" here):

abstract class ContentEntityBase {

  /* ... */

  public function &__get($name) {
    $context = \Drupal::service('entity.contextual_data.ephemeral');
    assert($context instanceof EphemeralContext);
    $data = $context[$this];
    assert(is_array($data));
    return $data[$name];
  }

  public function __set($name, $value) {
    $context = \Drupal::service('entity.contextual_data.ephemeral');
    assert($context instanceof EphemeralContext);
    $context[$this][$name] = $value;
  }

}


// ----------------------------

$node = Node::load(1);

// These throw 11.x -> 12.0 deprecation messages.
$node->foo = 'bar';
$bar = $node->foo;

// These throw 12.x -> 13.0 deprecation messages.
\Drupal::service('entity.contextual_data.ephemeral')[$node]->set('foo', 'bar');
$bar = \Drupal::service('entity.contextual_data.ephemeral')[$node]->get('foo');

Thanks for the feedback :)

@catch:

Yep, and of course we have field widgets and formatters that have a similar isApplicable method. The main difference is that it's static. Having to instantiate the plugin to check its applicability is by design in the PoC code, since the logic may require to do more than just evaluating definition values.

@berdir:

Tagged services seem sufficient for this, breadcrumb builders is a valid example, so is for example the theme negotiator. It's pretty well standardized using the service_id_collector tag, see theme.negotiator and services tagged with theme_negotiator. Also supports priority

Good point. I think there was a similar conversation when discussing the implementation of the language negotiation system: in the end we went with plugins but tagged services would likely have worked just as fine. At the time I was concerned with not burdening the DIC with too many services, but this is less of a concern.

@larowlan:

One thing plugins have that tagged services don't is derivatives.

Another good point :)

I definitely leveraged derivatives in combination with applicable plugins in one of my client projects. Being able to write custom logic for each plugin implementation but also to rely on derivatives for simpler strategies varying only by some parameter value (stored in the plugin definition) was definitely useful.

Which is my main objection to @berdir's point:

A primary reason for using plugins is to manage multiple instances and configuration for them, something that by definition isn't really needed for first-applicable-implementation wins.

It's definitely true that I never relied on plugin configuration in the client projects that are using the PoC code :)

@fabianx:

So we have two things to take into account here:
a) Ensure that properties survive a reload of the entity (for certain cases, others are relying on the exact object instance).
b) Ensure that a diff of original entity vs. modified can still be retrieved somehow.

IMO a) could be solved by having a contextual data storage relying on \SplObjectStorage or \WeakMap. Depending on the use case, one could instantiate an EphemeralContext class (relying on \Weakmap) or a PersistentContext one (relying on \SplObjectStorage).

I think we have (at least) one issue dealing with b) ( #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) → ), but, if we go with a separate contextual data storage, we don't have to worry about that here :)
Actually, a lazy builder could serialize the entity contextual data, which should be far cheaper than serializing the whole entity.

Maybe introducing a distinction between mutable and immutable entity objects, like we have for config, would help there (and would probably be desirable for other reasons), but that's definitely not a small undertaking :)

I think the problem we have is that the entity has a fixed interface and we want / need to enhance that interface during the runtime.

IMO this is exactly where the distinction between legitimate entity data addition and contextual data lies: (content) entity data should live in fields (stored or computed). When additional fields are defined that the base interface does not know about/support, you can always rely on decorators/wrappers to access them and benefit from static analysis and IDE autocompletion. The Content Translation module follows this pattern with its ContentTranslationMetadataWrapperInterface.

OTOH ephemeral data without a proper definition, currently stored as a random property, only makes sense as contextual data and should not live on the entity IMO.

@berdir

That said, the simple use case for a single loaded entity that should explicitly _not_ persist across clones and static cache clears is IMHO somewhat valid, and if we want to deprecate magic get/set then we need at least an intermediate solution for this.

I agree, but my concern is that, by adding an API to support that use case on the entity class itself, we are encouraging people to take advantage of this pattern even more, which is likely to result in abuse, since it's hard to tell a legitimate API usage (very few IMHO) from an invalid one (most of them IMHO 🙂).

Some things like isPrevie should be handled by the specific entity type as it's a feature they provide [...]

💯

what about _referringItem, set during rendering an entity reference?

Yes, this is a clear of example of an ephemeral/contextual property: a file can be referenced by multiple entities, so adding an explicit parent reference would make no sense and would not help here. IMO the right way to handle it would be something along these lines:

  /**
   * This is an alternative version of EntityReferenceFormatterBase::getEntitiesToView.
   * It is passed a Context object previously initialized with field item
   * contextual data (specifically the "_loaded" property). Additional contextual
   * information is added to it as the entities to be displayed are collected.
   * The Context class would implement \ArrayAccess and likely wrap a
   * \ WeakMap instance.
   */
  protected function prepareEntityDisplayContext(EntityReferenceFieldItemListInterface $items, string $langcode, Context $displayContext): array {
    
    $entities = [];

    foreach ($items as $delta => $item) {
      // Ignore items where no entity could be loaded in prepareView().
      if ($displayContext[$item]->get('loaded')) {
        $entity = $item->entity;

        // Set the entity in the correct language for display.
        if ($entity instanceof TranslatableInterface) {
          $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity, $langcode);
        }

        $access = $this->checkAccess($entity);
        // Add the access result's cacheability, ::view() needs it.
        $displayContext[$item]->set('access_cacheability', CacheableMetadata::createFromObject($access));
        if ($access->isAllowed()) {
          // Add the referring item, in case the formatter needs it.
          $displayContext[$entity]->set('referring_item', $items[$delta]);
          $entities[$delta] = $entity;
        }
      }
    }

    return $entities;
  }
  

The $displayContext would be passed around as needed and discarded once the render array is returned. No need for "persistence" at all in this particular case, I think, as all the consuming code is in the formatter classes extending EntityReferenceFormatterBase (at least in core).

There's the ->view property that's set when an entity is rendered within a view.

Another contextual property, I assume something like this would be viable in both the the occurrences I found:

  public function preRenderByRelationship(array $result, string $relationship): void {
    $view_builder = \Drupal::entityTypeManager()->getViewBuilder($this->entityType->id());

    foreach ($result as $row) {
      if ($entity = $this->getEntity($row, $relationship)) {
        // Remove this: $entity->view = $this->view;
        $this->build[$entity->id()] = $view_builder->view($entity, $this->view->rowPlugin->options['view_mode'], $this->getLangcodeByRelationship($row, $relationship));
        $this->build[$entity->id()]['#view_executable'] = $this->view;
      }
    }
  }

Storing that elsewhere requires some workarounds like storing it by object hash which can have weird edge cases (the documentation on that explicitly mentions that hashes may be reused) [...]

I believe using a WeakMap would solve this: the entity contextual data would no longer be available as soon as the original entity object were destructed.

plach → created an issue.

I think the persistent thing adds a fair bit of complexity that is somewhat unclear just how persistent it is?

They way I interpreted it, it seems this the OP is taking about persistence throughout the request/response cycle. From time to time, you need to "persist" information between two points of the execution flow without no clean way to pass that around. In the dark days we would use a $GLOBAL variable for that, but for one that's compatible with the request stack. A saner alternative could to be to attach attributes to the request itself, but that may force you to inject the current request or the request stack into a context that has nothing to do with those concepts, which is suboptimal as well.

Granted that the need to pass data around like this is itself not ideal and likely an indication that something is wrong in the subsystem relationships or the logic being implemented, I tried to address this use case for a few custom projects of mine through this code, which is close in spirit with what was proposed in #8. Something like would support both use cases.

Personally, I'd like to drop entirely the habit of setting random data on entity objects.

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.