🇩🇪Germany @geek-merlin

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!

Yay, this impulse may set some energy free in the community. For me it certainly does.

Some thoughts:
1) For all i know, objects having performenca penalty is long ago (php5?).
2) I'm maintaining RenderArrayTool Library → and have gotten to love that approach, for building, but much more for altering render arrays.
3) So this is an interesting approach: to have the Renderer eat render arrays OR RenderableObjectInterface. If the render elements then have typed properties, altering would be a lot more fun.
4) Another win of this approach: It would finally allow evolution. A RenderableObjectInterface (which is different to RenderElementBase or its interfaces) is something that spits out a html string in the end, BubbleableMetadata (roughly cacheability plus libraries) and may or may not use a render array in between (so it is also not RenderableInterface).
5) Hacking together a ComponentRenderElement would be trivial.
6) A RenderableFormElementInterface for form elements would need a bit more bells and whistles, as it has to interact with the form workflow.
7) We may even have a DrupalFormElement and other form elements, which would allow using other form libraries like symfony forms (though i strongly doubt they will ever be interoparable.)

8) As of the API: There are lots of properties and methods (especially for forms) that have no relation to the DOM. But without doubt, having the dom related methods resemble DomExtension, has a lot of charm.
9) Yes, we don't want to extend DomNode for a variety of reasons. Whether the New-Dom empty marker interfaces \DOM\ParentNode and \DOM\ChildNode should play a role here, is up to debate.

So in the end, for me it boils down to opening a core issue like "Add RenderableObjectInterface and allow it in render arrays".

WDYT?

geek-merlin → made their first commit to this issue’s fork.

Thank ya all! So fixed.

It's a feature, not a but that this is a library (so no type declaration needed). Added a sentence on the module page.

Thx for elaborating. To clarify:
- This is abolut "single" access for a page manager page.
- It is not related at all to any views "list" access.

There are lots of good lazybuilder docs and articles, and examples like this. You should look how far this gets you and can pm me with an MR.

Yup.

LGTM too.

Already fixed in HEAD.