Freiburg, Germany
Account created on 17 January 2008, about 17 years ago
#

Merge Requests

More

Recent comments

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

Threw it out.

🇩🇪Germany geek-merlin Freiburg, Germany

geek-merlin created an issue.

🇩🇪Germany geek-merlin Freiburg, Germany

Maybe sth like this works:

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

🇩🇪Germany geek-merlin Freiburg, Germany

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?

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

Also, cashe item lifetimes might prevent flushing.

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

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!

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

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.

???

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

geek-merlin created an issue.

🇩🇪Germany geek-merlin Freiburg, Germany

They are separate entities and will always be.

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

Can you share your approach?

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

Maybe a better IS helps.

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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?

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

Looks like tests are red..

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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?

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

Thank ya all! So fixed.

🇩🇪Germany geek-merlin Freiburg, Germany

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

🇩🇪Germany geek-merlin Freiburg, Germany

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.

🇩🇪Germany geek-merlin Freiburg, Germany

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.

Production build 0.71.5 2024