🇩🇪Germany @geek-merlin

Yes, obsoleted.

@wim: Can you clarify: Is "array" a dict, a list, or any of it? PHP lenience...

Bot has some cs nits:

FILE: ...pal11/core/tests/Drupal/KernelTests/Core/Field/FieldMappingStorageTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
19 | ERROR | Missing member variable doc comment
29 | WARNING | t() calls should be avoided in classes, use
| | \Drupal\Core\StringTranslation\StringTranslationTrait and
| | $this->t() instead
--------------------------------------------------------------------------------

Habemus icons yay!

Since TrustedCallbacks → , sth like this does not work anymore:

    $element['#pre_render'][] = [HelpCallbacks::class, 'preRenderHelp'];

Fix is:

    $element['#pre_render'][] = HelpCallbacks::class . '::preRenderHelp';

Thanks @ and @kristiaanvandeneynde for the good discussion in #242 ff!

Twice 5 cents on this...:

@davidwbarratt, would you like to review the API i pioneered some time ago in Entity Unified Access → , how it resonates with your vision? (Feel free to PM me on this.)

@kristiaanvandeneynde, I have a lot of resonance with the limitations you point out, i have similar experiences. But with all due respect, imho you are wrong that an access alter should be able to take the role of the aggregating AccessControlHander. I know you had to implement group access control that way and all respect for implementing it in the current API. But i think what you point out is the result of a lack of a pluggable access control aggregation api like symfony has.

I took some time to think this over and text it in 📌 Add a a modern EntityAccessAggregation API, and kill 3-State AccessResults Active . The above discussion indicates to me that this issue should be postponed on the other.

geek-merlin → created an issue.

Imho this is obsoleted by the long-fixed related issue.

geek-merlin → created an issue.

geek-merlin → created an issue.

> Are most sites only using one relationship on an entity?

Yes, i suppose.

And: Nice to have you on board here! Did you review or test my approach?

This does the trick for me. Tested on a site with hundreds of autoconfigured hook services. Also fixes the related issue.

geek-merlin → created an issue.

geek-merlin → created an issue.

But rolling another alpha for now, and let's wait some time for adoption.

This is nice to have but no blocker either.

This seems no blocker to me.

geek-merlin → created an issue.

Already fixed. Thx for reporting!

Thank ya all!

LGTM.

Thx to all!

LGTM.

Thank ya all!

Special thx to @joelpittet to notify the feature creel in ä10.
Technically, it's also addressing the related issue, but let's geit it in and call it a day.
Not adding #14 though.

MR lgtm.

Pastch #2 lgtm.

Kristiaan, my first impression: i like it a lot where this is going!

Thought a bit about this, and yes, the "domain group preset" case and similar ones are handled nicely.

There are sites though, where the group must come from the URL, but this can be handled by sth like /node/add/foo?group=123

Then there are the use cases where group must be selected on the /node/add/foo page itself.

Which rang a bell:
Why not suck in a stripped down version of Entity group field → ,
and let widgets (like fromDomain, fromQueryString etc) do the work?

(Without that la last use case, which we had quite often, might turn out a PITA.)

Yes!

This is trivial and should be merged.

The MR contains changes not related to this issue.

Adding stable patch for composer patching, please ignore for all other purposes.

Yes, i'd like that a lot too.
But no, the current scope is way too big.

How i see it reasonable:
- Take DynamicEntityReference (entityTypeId, entityId) out of DER module → , including views integration
- Add an extended DynamicEntityReferenceSource (entityTypeId, entityId, sourceFieldName) field type like paragraps parent does, add views integration
- Add a source / target table, and a way to opt reference fields into usage tracking
- Do NOT support for non-integer IDs in the first step (see below)
- Do NOT support non field usages in the first step (to be added later via special sourceFieldName values)
- Do NOT support displaying the usages (it can be done via views)
- Do NOT support transitive relations (e.g. node -> media -> file) in the first step (see below)

Please open a core issue to get the required API and postpone on that.

Great stuff! Has this a chance of being backported? (Sorry i did not find the criteria but saw that other navigation top bar stuff is.)

Threw it out.

geek-merlin → created an issue.

Maybe sth like this works:

  public function destruct(): void {
    if ($this-inner instanceof DestructableInterface) {
      $this->inner->destruct();
    }
  }

I can confirm this (also see broken translation_bliss pipeline).

The patch looks solid and is green. So RTBC for 1.7.

This was a PITA in the first place and is more of that now, dunno what to do with 1.6 for both d10 and d11.
But targeting pre-change d11 and post-change is the same problem.
Is this core change in a release yet?

What about starting with Option one and doing the others in a follow up?

This is still relevant. Of course the code need rebase but it is a treasure and should not be burried in a closed issue.

Also, cashe item lifetimes might prevent flushing.

Yep, redis needs the extension *in the phpunit job*, but ymmv...
My prob was different and now sessionless may serve as another example to link to.
But i don't want to spread more half-baked knowledge, please let people who know better write that doc.

I burnt some hours to advance from the blind to the one-eyed, but not i got it.

geek-merlin → created an issue.

Thx @fjgarlin for acknowledging the problem is a problem ;-).
Still struggling with it, and opened a docs improvement issue.

geek-merlin → created an issue.

geek-merlin → created an issue.

The provided patch is as wrong as can be. But was already fixed.

Did a code review of the MR.

The factored out ::buildCreateAccessCid lgtm, it does what it announces, and defaulting complex cases to uncacheable seems reasonable.
Test coverage looks reasonable, though i did not think about each case. (Only found an Übernit in the code.)
Setting RTBC. You should revert though that if you think that more thorough test review is needed.

So it's up to the AccesControlHandler to add all "possible" $context variations to $cid. And if other things are passed in $context and used in an access hook, that is not supported (which may go to the docs...).

It's not my intent to bloat this, just to clarify.
Thanks a lot for pushing this forward!

When access is calculated and altered in hooks, any number of additional cache contexts that can not be known in advance can be added.
So apparently VariationCache is needed for that.

Thanks for the reply. You are right, mostly.

But what about this: the $context (and things derived from it) CAN be used in hook_entity_create_access_alter, with no way to broadcast that fact back to the AccessControlHandler.

???

This needs a different approach.
The access result can vary by all sorts of context ($context, dereved from it, and external ones). This is the role of cache contexts to be returned with $accessResult (maybe from a hook_access_alter). These cache contexts can not be known in advance.
Sounds familiar? Yes, this is exactly what VariationCache is for.

geek-merlin → created an issue.

geek-merlin → created an issue.

geek-merlin → created an issue.

They are separate entities and will always be.

Updated reply to @wimleers' comment regarding security of unserializing:

We already have that unrestricted unserialize in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapFromStorageRecords today.
So if we need security hardening here, it's a separate issue.

That said, after some thinking i share the concern about API-injected evil classes into field data. Thinking about where to open that issue.

As of @wimleers' comment regarding security of unserializing:
SA-CORE-2019-003 → is about user-entered form data.
This unserializes app-controlled data from the DB, the same as in \Drupal\Core\Cache\DatabaseBackend::prepareItem.
(Adding a comment to ALL such places makes a lot of sense though.)

@huzooka #14:
> Drupal core does allows non-integer IDs, so we used this approach:

Can you share your approach?

FTR: have the same error message, but debugging shows that in my case another plugin was to blame: Drupal\commerce_order\Plugin\EntityReferenceSelection\UserSearch

After applying the same change as above, the problem is fixed.

Will file this against commerce.

Maybe a better IS helps.

@smustgrave
> Just to be safe though I wonder if we should add a simple deprecation that triggers if someone passes in the old type. Just to be safe.

Sounds like sth got confused: Is so "passes in the old type" (LazyContextRepository), it is always an instance of ContextRepositoryInterface (the "new type") and will always be. So nothing to deprecate here.

RTBC as of #6.

I doubt that i'll have time soon-ish, but how i'd approach it:
- In ServiceProvider, register a CompilerPass
- In CompilerPass, iterate classes in the blessed directory, check attributes, then set the collected classes as a constructor parameter of BundleClassAltererHooks service
- In BundleClassAltererHooks, alter the bundles

Trivially rtbc.

Also, after having groked hook attribute discovery in a compoler pass, this should also run in a compiler pass (not as plugins).

The link in #9 is an example commit showing how this feature can spare boilerplate code in other modules.

The MR is totally messed up. Already fixed in HEAD though.

Your comment is not helpful for me to understand what you want. what is your experience, what do you like to see what is the difference?

Resolved und tagged 5.5

Great news! For me this would be enough to say this is fixed

Looks like tests are red..

geek-merlin → created an issue.

> registerAttributeForAutoconfiguration is only used by the full Symfony framework, not the components as shipped and I am fairly sure it requires the Symfony Config component.

Hmm, i am using it i a quite big customer project with >300 contrib and custom modules to autoregister hux classes. No package added, except ~10 lines of custom code, and no extraordinary container build times.

Simple and more robust patch flying in.

Patch URL: https://git.drupalcode.org/project/field_formatter_class/-/commit/e17b4c...

Patch applies cleanly on my site and fixes the issue for me.

geek-merlin → changed the visibility of the branch 3473891-undefined-method-getEntity to active.

geek-merlin → changed the visibility of the branch 3473891-undefined-method-getEntity to hidden.

Ran into this too. And must be solved more robust.

> but it effectively just does the reflection looping that is the performance issue in question.

Which means:
- That compiler pass is already in place, so no nightmare to fear.
- It's the central place to do the reflection, with no performance penelty for any additional consumer.

So IF that is used, no reason for performance fear left.

geek-merlin → created an issue.

@ghost of drupal past #11, @berdir #16, et al

it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute

Maybe you are not aware of $container->registerAttributeForAutoconfiguration?
See #10 on how to implement.

Yup, thx!