🇩🇪Germany @geek-merlin

So...

Yup, now we're green.

It's the menu entity type that coughs, seems like there is a definition for it even before the module defines it. Anyone knows what that's about?

But no problem, we can fallback to the provider condition.

OK, this breaks some tests.

Got some insights. Working on it.

Yes, doing so.

Made the title more general to not provoke more dups.

Here we go with an updated IS. Which should make clear that this is not a drush issue.

Woot! Great pointer.
Yes, i will bring that other issue a bit in shape.

Crosslinking a regression.

Crosslinking regression

geek-merlin → created an issue.

geek-merlin → created an issue.

Rebased. Did not investigate why this fails though.

Rebased. Due to the broad changes in Importer.php i'm not totally sure but quite confident i resolved the conflicts correctly.

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

Maybe work for Rector?

Thanks for bumping this and doing a manual test.

- Can you please add a MR, so we can see what the testbot says?
- It looks like the patch changes the resulting array shape, maybe that does not matter, maybe it triggers regressions elsewhere.
- Can you put your manual test into an automated test? My feeling is this really needs a test.

Thanks a lot for the pointers!

> prepareView() and buildMultiple(). It's incompatible with render caching

The linked issues only say render caching has to be adjusted for that.

As @yched points out, it's a question about cold cache performance, critical in high-cardinality environments:
#2843565-15: getViewBuilder('node')->viewMultiple() bypasses render cache →

On the overall "entity render cache kills multiple-entity view building" issue :

We kind of accepted that tradeoff after we discovered it, on the grounds that the multiple-view optimization we lost only impacted render-cache misses. But that's assuming there is a reasonably high hit ratio on entity render cache :-)

If I'm not mistaken, that assumes :
- uncacheable / high-granularity formatters do the right job of being placeholdered
- the entities' cacheability (and notably the cacheability of 'view' AccessResults) is not too granular either.

Anecdotical evidence: I worked on several projects with such high-cardinality views. So my experiance is not "that's neglegible".

@mondrake Thanks a lot for confirming it is an issue and a variant of phpstan-ignore is currently the only way out.

And thanks for the pointer, maybe you want to boost that by confirming my "what would make sense" over there at #3376518-190: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough → .

geek-merlin → created an issue.

geek-merlin → created an issue.

Really nice! Inspired me to create the related issue. I guess both of you @catch and @berdir have some thoughts on it.

geek-merlin → created an issue.

geek-merlin → changed the visibility of the branch 11.x to hidden.

Wow @mfb, this was a quick implementation.

Reviewed !14205 and it's exactly like I imagined in !97. Current tests are green.
I'd RTBC this, but it has the Needs-Tests tag.
Also yes, this must wait for the closure-serialization issue. Added it as related.

MR LGTM, tests are green.

@stoeckler Thoroughly debugged, thx a lot! I especially like the explicit reasoning.
And here is where i disagree on a small but crucial assumtion:
> (3) The problem is that with the explicit setConfigOverrideLanguage() API there is no way to detect if a language has been set explicitly or if it is just the fallback.

In LanguageConfigFactoryOverride we CAN negotiate the configOverrideLanguage on demand instead of relying on the explicitly set value, and ONLY fall back to default language on recursion. Some while ago i implemented this in a project and never found time to upstream it. I will give this a MR shot soon-ish.

Code looks good and tests are green, so i'd RTBC this, save #21 "persisted" bins.

@pfrenssen Can you elaborate what this "persisted bins" is used for in the first place?
IIRR, core has a persist flag for individual items, but it's never a guarantee as of LRU deletion.

I'm running into such circular refs twice a year and every time it's like here a "fat constructor" with a config-get-call. I'm with @berdir that the proper fix is to move the call out of the constructor, and in all the cases i had that solved it.

Maintaining BC works w/ a __get facade. And we even don't have to deprecate it, just replace it w/ a PHP8.5 property getter hook in D12.

Another dup of this: https://www.drupal.org/project/filecache/issues/3129225#comment-16396609 💬 [WORKAROUND] Another (different) circular reference detected between filecache and syslog Active

It is that core issue, postponing on it.

We ran into this too on first deploy, it's a big PITA. Thx for the workaround.
Setting major task for this.

Yah right! Merged.

Updated. Feel free to suggest improvements in a followup.

The new commit adds the #14 composer.json line.
Nothing else changed, so still rtbc.

No changes, still rtbc.

Yup, i noticed that, and it is very good. I've just seen that an awful lot of complexity has been added to the system to eradicate single cache tags, and wanted to provide an alternative, to help keep code maintainable, have no more complexity introduced, and maybe roll back some complexity and use cache tags again. Apart from that, i have no interest in pushing this solution further.
So if someone wants to pick it up and do some benchmarks, fine, if this rots forever as POC, fine for me too.

geek-merlin → created an issue.

@catch #9: Interesting, but this is NOT the main use case.

Let me make a use case on a custom site:
You have a site with 80 node types, and core maintains 1 list cache tag for each and every one of them. Maybe you don't care at all. Or maybe you want one cache tag for 3 of them and a second cache tag for the other 77. The POC aggregator lets you do that with minimal effort.

The big picture: We have those constant wars on more cache tags here, less cache tags there.
Some developer use cases profit from more fine-grained cache tags (in the API), and in the case of bundle list cache tags, core said yes.
But for performance, more cache tags (in the DB) are bad.

The approach allows to have the cake and eat it. And to stop that war:
Add more cache tags in the API and make developers happy. And to map them to a smaller set in the DB to make performance owners happy.

Maybe this makes it clear.

@godotislate #48: [Proposal to not support service-with-hook decoration.]
The idea to limit scope sounds reasonable. TBH i do not understand the ramifications or alternatives of this (maybe i missed a relevant comment when vgrepping.)

I see two different cases:
a) One foo.service both implentis an interface AND implents a hook (not being part of that interface). While i would NOT recommend this for "clean-ness", it totally can be done. Another bar.service decorates foo.service interface methods, not caring for the hook.
=> My expectation is that the hook still runs in the (now renamed) bar.service.inner (ex foo.service).

b) One foo.service implements (say) hookEntityPreSave, and another component wants to wrap that hook.
=> This has relevant complexities, and it has my +1 to say it's unsupported and defer it to a separate issue "Allow hook implementations to be wrapped".

Some thoughts on b)
- Hux has a dedicated attribute for it.
- My gut feeling is not NOT support decorating the hook-implementing service, as it may contain multiple hook implementations. And it's not a well-defined interface that we're decorating, but the "interface" is a single hook implementation and its signature, NOT all public methods, because Hook service classes tend to have hook implementations added over time, For the same reason, let's stick with the term "wrap a hook implementation", because wrapping a hook implementationis technically not decorating.

@godotislate Is this along your lines of thought?

Boldly pushing a distanced view on this into the discussion. (Pleaso boldly revert if i missed something.)
(I like the idea of moving components to vendor and configuring what is scaffolded public, but let's decouple that to later bikeshedding.)

@godotislate Nice that you bumped this, it reminded me to push an approach from my mind's idea pipeline.

In short: Marking the service at the source is dead simple. And: ServiceIterator needs the same (ServiceLocator not), so maybe rename and reuse the attribute.

Great! Setting RTBC as of #10 plus test-only result.
The new test fails as expected.

(Still wondering why i could not trigger the test-only run myself...)

Note on the failing test: That test shows that the existing ArgumentResolver code throws a rather obscure error on union types.
Nice that that one is fixed by this MR too!

Reviewed the MR.

It looks good and does what it announces:
- It adds the library
- leverages it in ArgumentsResolver the way expected
- adds a test that elegantly covers both code paths, scalar and object

Yay, i waited a long time for this one.

I'd set this RTBC, but one check is missing:
- [ ] Test-only is red

For some reason i seem not to be permitted to start the test-only manual action (both ones, and yes, i'm logged in, and tried to re-login.)

So if that is checked, it's RTBC.

@ishani patel, @jlockhart: As explained in #10 and noted in the "Needs test" tag, this issue is waiting for someone to write an automated test.

geek-merlin → changed the visibility of the branch 2903964-nesting-paragraph-into to hidden.

Is there even something left to do here?

#5: Thx! +1 on that.

Use case and reasoning of the issue summary is still relevant.

@davidiio: Sorry, "Needs tests"...

Answering my own #7 objection (you know me, i don't believe anything that i can not prove):
>(2) How and why can we guarantee that all entities saved are related to the moderated entity? Still thoroughly thinking about this.

This is nothing new, but a basic question for workspaces. HTTP mandates that app state only changes in POST requests. And we can assume that all entity saving in such a POST is related to the current form. If not (like poormanscron) they have to be workspaces aware and execute in null workspace.

Remedied.

Still the question of workspace payload for string IDs.

@mondrake: #100 to make all this thoroughly typed!

But kittens (and me) suffering a lot from the 'extra' keys bringing back the array-PI that was about to be eradicated in the first place.

class-string pattern to the rescue. I guess you saw it before:

In this issue it is implemented like this:

new Table(
  extra: [
    MySqlTable::class => new MySqlTable(
      engine: MySqlEngine::InnoDB,
      characterSet: CharSet::utf8mb4,
    ),
  ],
);

Keying by class-string allows PhpStan, PhpStorm, and friends to infer types:

  /**
   * @template T
   * @param array<class-string<T>, T> $extra
   */
// Class can now be final as $extra allows infinite extensibility.
final class Table {
  public function __construct(
    private readonly array $extra,
  ) {}
}

Consuming code can now do:

$mySqlTableExtra = $table->extra[MySqlTable::class];
if ($mySqlTableExtra) {
  // This is NOT necessary, because of class-string.
  assert($mySqlTableExtra instanceof MySqlTable);
  ...
}

Code is straightforward. Test is green. Test-only breaks the way expected.
Get it in!

@phenaproxima: Glad you understood my surprise ;-)
And thanks for the hooks-in-tests pointer, this is a life-saver!
And yes, "syncing" is an odd-nomer. See also 📌 [PP-2] Better describe how SynchronizableInterface should be used for content entities Needs work .

@alexpott: I can't self-RTBC, but code-wise this this is back to #37 which was RTBC.

Bump!

@amateescu Great work, rock on!

Found a bit of time to do a first round of review. And yes, found some architectural points.
One is the workspace payload, but this is remediable, which may or may not involve workspace bundle fields.
The other is conceptually way deeper. How and why can we guarantee that all entities saved are related to the moderated entity? Still thoroughly thinking about this.

PS: Review appreciated on 📌 Make provider the bundle key of workspaces (or document why not) Active .

Also when i cross-checked the above in the IS, the reasoning does not match the proposal anymore.

In the next step let me challenge the name-derived-from-type condition. What is it meeded for? If a parameter name is not derived from a type, what is won from duplicating it in a phpdoc.

I.e., what is won from

/**
 * @param AccountInterface $currentUser
 */
function(AccountInterface $currentUser)...

over

function(AccountInterface $currentUser)...

(If i missed the reasoning, let's at least add it to the IS.)

Forst of all, +1000 for updating code style that was created when php had no types.

The current MR does not make sense though:

A `@param` tag must have a description, unless ...
The parameter type is a single class ... [and] the parameter name is derived directly from the type name

(Omit the description if ... parameter name ...???)

What would make sense:

* Each parameter of a function must be documented with a `@param` tag (see exceptions below)

+* A `@param` tag must have a description, unless
+ 1. A description would not add useful information.

+ The @param thag can be omitted altogether if there the above is the case,
+ and also the phpdoc type definition does not add useful information, i.e.
+ 1. The parameter is type-hinted.
+ 2. The type in the `@param` tag is identical to the parameter type in the PHP function signature.
+ 3. The parameter type is a single class, interface, or scalar and does not allow null.
+ * For a class or interface, the parameter name is derived directly from the type name. For example, `QueueWorkerManagerInterface $queueWorkerManager`.
+ * For a scalar, the parameter name is self-documenting.

Original MR for reference:

* Each parameter of a function must be documented with a `@param` tag (see exceptions below).
+* A `@param` tag must have a description, unless all of the following are true:
+ 1. The parameter is type-hinted.
+ 1. The parameter type is a single class, interface, or scalar and does not allow null.
+ * For a class or interface, the parameter name is derived directly from the type name. For example, `QueueWorkerManagerInterface $queueWorkerManager`.
+ * For a scalar, the parameter name is self-documenting.
+ 1. A description would not add useful information.
+ 1. The type in the `@param` tag is identical to the parameter type in the PHP function signature.

@phenaproxima Seriously?? It's one of a gazillion "update copied code like the original" issues. Did you read the linked original?

Hi Andrew, yep a wholehearted "welcome aboard" from me too!
(I'm not fast with reacting to PM but will do!)

Hi Andrew, yep a wholehearted "welcome aboard" from me too!

I'm needing this for my navigation breaking when an entity returns markup, so rolled it in code and attaching a snapshot for composer.

Here we go: Initial 1-attribute approach, verbose phpdoc. CR updated too.
Tests are green, test-only red the way expected.

Thank you all for your feedback, and for saving me from barking up the wrong tree ;-). Partial revert is coming.

Hmm, i see your point!

So we have:
- Verion 1 - only TwigAllowed
- Bad: In the first step, whitelisting for a class can be a mixture of (legacy) Magic and explicit Attributes
- Good: Only one attribute, only one deprecation step
- Verion 2 - TwigAllowed + Class attribute
- Good: The class attribute makes it crystal clear if legacy Magic OR explicit Attributes rule
- Bad: One more deprecation step

I don't have strong feelings.
@alexpott, do you have?

@dpi Renamed the compiler pass names.
(Note that this obfuscates how trivial the first commit really is, so you may want to read the commits separately.)

geek-merlin → created an issue. See original summary → .

This still makes a lot of sense, but more as an evolution than revolution.
Benevolently hijacking this.

And, I think this is in perfect harmony with the magnificient work in 📌 Slowly, very slowly start OOPifying the render system Needs review .

geek-merlin → created an issue.

Ups, of cource technically the changes need review.

Added followups, postponed for now.

geek-merlin → created an issue.

geek-merlin → created an issue.