🇨🇦Canada @Charlie ChX Negyesi

Sorry bogus report

ghost of drupal past → created an issue.

The road forward is https://www.drupal.org/project/drupal/issues/3525331#comment-16227159 📌 Slowly, very slowly start OOPifying the render system Needs review and while I am quite sad no one picked it up, I can not do this any more.

How is it relevant to this issue? If people worked on this then by Drupal 13-14 the render arrays would be gone and markup would be a property.

That would be the most immediate introduction of using the resolver and wiring up arguments from the environment into the application.

Are we sure we want to have the debate here and now? I suggested to do it when the need arises. But fine. Here:

$environment =  $_ENV[whatever] ?? 'prod'; // This goes into the template
$app = new DrupalKernel($environment, require 'autoload.php', TRUE, $context); // this is the front controller

It can't really be anything else because there are no extension points available here yet. All that you resolve must come from the superglobals so why not put the superglobals where they belong especially when you already need to write custom code to get the value out of the superglobals ? Or maybe there can be some composer overrides -- but the template is written out from composer so just read that in the plugin. But, really, there's nothing else.

I maintain there's a very strong emphasis on need. I see no need. Maybe if this issue was about writing symfony/runtime itself then I could be convinced this is necessary somewhere down the road but that's not the issue. You just need to get Drupal running.

Now, the reason I didn't want a custom runtime and runner because then it would become possible to use https://github.com/php-runtime/frankenphp-symfony directly (and possibly others). HttpKernelRunner doesn't change the behaviour, those fastcgi etc lines are copied from Response::send(). So then let's perhaps keep DrupalRuntime for the constructor override alone but skip DrupalKernelRunner? This allows using the frankenphp runtime as is -- once Drupal is reentrant at least -- while avoiding the problematic constructor. See, I am actually not blindly against Symfony. Let's use what makes sense.

Have you tried to step through booting in the new codebase? I did.

Let me simplify the concerns:

1. Yes, symfony/runtime is needed. There's no question -- alas.
2. The default code flow of symfony/runtime is way too complex and absolutely not intuitive.

Specifically, here's how I would imagine a vastly simplified front end controller:

$application = new DrupalKernel('prod', require 'autoload.php');
(new DrupalRuntime())->getRunner($application)->run();

What am I missing?

QueueServiceInterface is in the purge module. If I were still a queue maintainer I would ask the purge maintainers whether they need this (but the module haven't since a commit since 2024).

-- small footnote, no idea where to put this, the issue reminded me: the handbook page lists pwolanin as book maintainer only since 8.0, he has been the book maintainer de facto since he rewrote it in 146425 in 2007 for the new menu system he and I did so that's drupal 6.0. He was officially only added in 2010 in the big maintainers revamp in 621618 but that's just an oversight.

Erm what.

1. I missed the memo when upsert got added. I would've protested it. That's on me.
2. Indeed with keys added we are now at a point where MERGE and UPSERT functionalities overlap so much that MERGE can just fall back to an upsert if the more complex methods are not called.
3. Why on earth would you drop MERGE when finally support is arising? I wrote "Other databases (most notably MySQL, PostgreSQL and SQLite) will emulate this statement by running a SELECT and then INSERT or UPDATE" so many years ago when life was worth living but by now this is not true as PostgreSQL 15 in 2022 added MERGE support.

tl;dr: drop UPSERT, make MERGE smart, rejoice.

Let this be known now: the only reason I used the event dispatcher and also the reason I added the possibility of using Hook on classes was because that's what Symfony does and you can short circuit a lot of debate by saying "Symfony". In my experience many people automatically presumes that's the right way to do things when it was proven repeatedly the exact opposite is true. Yes, this is a very bad way of doing things but I have not invented this anti pattern. Y'all made the rules of this game, I just played it. Do consider removing Hook from classes as well, it makes no sense.

However, I never dreamed of not needing to put the implementations in the container. I thought TaggedHandlersPass will be used to remove the event dispatcher since it has the exact same functionality as the event listener registration pass. This is an very unexpected, super nice optimization berdir found, thanks.

As the author of some of that code: you could also delete some code after forking, after all Drupal only ever supported class annotations in proper PSR namespaces.

It's one of those things that would be good but so minor no one actually picked it up. Is there anyone doing guided contribution hours? It's not too bad for a first issue although certainly bigger than just fixing a minor typo in docs.

Is this a duplicate of 🐛 Entity queries querying the latest revision very slow with lots of revisions Needs work ?

Is this a duplicate of 📌 Optimize joins and table selection in SQL entity query implementation Needs work ?

If I were still doing core issues I would close this as a duplicate of 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work

ghost of drupal past → created an issue.

Is this a duplicate of 🐛 HookCollectorPass fails to correctly register module hooks in some contexts Active ? If so, I would recommend closing this and reusing the title in the older issue because this title is much clearer.

I recommend closing this, it would make the installer even worse and it's bad enough already and packaging can very easily write the version either using regular expressions or just throw nikic/php-parser at it and be done in three lines of code. We do have a Drupal pretty printer package for it now -- it always existed in api module but yours truly factored it out into chx/drupal-pretty-printer so the latter route is easy.

DrupalKernel::preHandle calls $this->container->get('module_handler')->loadAll(); .

This issue needs to be closed (works as designed) and every project hitting this needs to be fixed. Calling hooks before preHandle is just not supported. It is called from the KernelPreHandle middleware, it has a priority of 100, middlewares with a weight of less than 100 will be fine -- although it means they run after page cache. If drush server skips it but calls hooks then ServerCommand is broken. It's really that simple. You can't call hooks when preHandle didn't run.

1. Maybe consider adding a few words to the CR as to why? "Drupal by default was committing a database transaction when its Transaction object goes out of scope, during the Transaction object destruction." and then add something to the tunes of "This causes problems with unpredictable object destruction order especially under xdebug 3.3+ in develop mode" because at first glance it's not at all understandable why such a boilerplate-y thing needs to be added -- most code doesn't want to return its transaction, destruction seems to be the right time to do it.
2. David nuked direct commit in #301049: Transaction nesting not tracked by connection → were those problems considered?

These days I dislike commenting on core issues and I dislike controversy even more. I read the issue but I absolutely can't find why a constructor argument was not considered. I might have missed something -- even the original issue summary (yes, I read that too) wanted a separate attribute but why? Of course it will work but I do not see how it's a separate thing when HookBy can't appear without Hook. To me that says they shouldn't be separate.

Of course, what do I know.

Anyone wanting performance have moved from dblog to syslog or other long ago. How long? Well, I raised this to critical eighteen years ago. This is safe to close.

I am guessing a cycle detector constraint would indeed be helpful. And it's time: Drupal 11 requires MySQL 8 which supports CTE. Using that it should be relatively easy to write a SQL query to detect cycles.

I am thinking something to the tunes of -- untested of course:

WITH RECURSIVE cycle_check AS (
    SELECT entity_id AS start_id, entity_id AS current_id, parent_target_id
    FROM taxonomy_term__parent
    UNION ALL
    SELECT cc.start_id, t.entity_id AS current_id, t.parent_target_id
    FROM cycle_check cc
    INNER JOIN taxonomy_term_parent t ON t.entity_id = cc.parent_target_id
    WHERE t.entity_id <> cc.start_id
)
SELECT 1 AS has_cycle
FROM cycle_check cc
INNER JOIN taxonomy_term_parent t ON t.entity_id = cc.parent_target_id
WHERE t.parent_target_id = cc.start_id
LIMIT 1;

Opsie. We forgot to write row plugins? Oh well. If no one missed them these last twelve years then it's safe to presume they are not needed and the single value object suffices.

The constructor says "entity_field", that's just outdated, it should be field eg.: core/modules/text/src/Plugin/migrate/field/d7/TextField.php

> migration plugin derived from a migrate_plus migration configuration entity

These are not supported by core. See https://www.drupal.org/project/drupal/issues/2462233 → for the start and https://www.drupal.org/project/drupal/issues/2625696 → for the implementation.

Since this module won't see a D11 upgrade now and also it's quite hard to uninstall a module which has been removed, I asked in #contribute how to proceed and the best I got was to release a new version with a hook requirements message to note this is the last version. Could you release another D10 module here with that?

It is.

Thanks for the great work.

Does this (or perhaps a previous one? doesn't matter) need a change notice for proxies need to be configured so that the HX-Request header gets passed to Drupal.

Since I edited my comment above, let me repeat: it's very likely the upgrade is just documentation. It just needs to be written and tested -- manually. No need for a phpunit test for that. Thanks for modernizing this.

If you can make upgrade work feel free to sunset this module and recommend config_shard instead. Likely it needs two deployments, the first installs config_shard and the second removes pax. I can't see it working in a single step. This, of course, would require config_shard being able to read pax sharded config dirs. It's very likely the upgrade is just documentation.

What I really want to avoid is having two modules for the same purpose especially when one is a dirty unmaintained hack. Which name they live under, I couldn't care less.

You are now a maintainer of pax. Please open a new branch and submit the module into there. A better implementation is welcome. No need for a new project. The current pax implementation is not worth maintaining if there's a better solution.

ghost of drupal past → created an issue.

What can be salvaged? Properties and magic set/get except the elements array is used to initialize, not a reference. Use it more strategic not so general. This here:

    $formObject = $this->elementInfoManager->fromRenderable($form); 
    $widget = $this->elementInfoManager->fromRenderable($element, Widget::class);
    $element = $this->singleElementObject($items, $delta, $widget, $formObject, $form_state)->toRenderable();

Should works fine without references. Inside you are in the "future" Drupal world where you deal with render objects and not render arrays. You can switch in your form alter to such and return back if you so want. But as long as the form and render system is concerned, nothing changes. It's just a helper for more convenient interacting with render arrays.

I am not going to work on this (or indeed, on anything else).

The easy way to use an issue fork is to use the chx/drupal-issue-fork composer plugin.

The CR is not clear to me. Is there a before/after of what should be done?

ghost of drupal past → changed the visibility of the branch 3538212-separate-dispatcher to hidden.

Amazing work, thanks.

But if this is necessary then what is a lazy service, wasn't that supposed to solve this? And if it doesn't, should the code setting that up be removed?

Based on the performance benchmark in the issue summary there is a 10% hit on the admin performance page from the Symfony event dispatcher just with standard without hooks. So I am not sure whether "hundreds" are even needed.

@znerol yes, it's possible this issue should be retitled to "revert the event dispatcher" and the issue title from here needs to move to yours.

I reviewed the parent issue and I am not sure what is going on, I am unfamiliar with the lazy notation, the dumper sets $service['lazy'] = TRUE; but the word lazy does not appear in Drupal\Component\DependencyInjection\Container. Is there a problem with that?

Not on cached pages. The performance regression exists on every page if you have enough listeners.

That's even better but a performance regression is present since 10.3 for any site with a large number of listeners regardless of cached pages. It's possible this issue needs to be retitled for that and the issue znerol linked is the urgent page cache performance regression which is independent after all.

Note the changes do not invalidate the performance work, they are cosmetic / build time. But now the code doesn't look like something the cat barfed up. That ladder of ifs was hard to comprehend, this is linear.

There. I reintroduced the container aware dispatcher but with much better tie-in via a new ContainerAwareEventDispatcherDefinition which YamlFileLoader uses. In the original version a new pass was needed instead of Symfony's. No longer. The very definition of the service captures the addListener calls from the Symfony pass and changes them into the argument structure required by ContainerAwareEventDispatcher. This allows keeping all the functionality of RegisterListenersPass including multiple event dispatchers with practically zero overhead. Tests could be resurrected of course and/or tests could be written for ContainerAwareEventDispatcherDefinition but since all of core flows through it, I haven't felt the need.

ghost of drupal past → made their first commit to this issue’s fork.

Nope, this will be a relatively easy and straightforward follow up to that, the phpstan/phpdoc-parser library we landed on does not support any tags except the phpstan ones which are very type oriented. I am adding support for the existing ones and it's one class with 1-2 lines of logic, usually one constructor line to define the properties and then a __toString to add HTML wrapper. Plus 3-4 lines of parsing logic but it's really formulaic, here's @section:

<?
protected function parseSectionTagValue(TokenIterator $tokens): SectionTagValueNode {
$sectionName = $tokens->currentTokenValue();
$tokens->consumeTokenType(Lexer::TOKEN_IDENTIFIER);
$description = $this->parseText($tokens, TRUE);
return new SectionTagValueNode($sectionName, $description);
}

class SectionTagValueNode implements PhpDocTagValueNode {
use NodeAttributes;
public function __construct(protected string $sectionName, public string $description) {
}
public function __toString(): string {
return sprintf('

%s

', trim($this->sectionName), trim($this->description));
}
}

?>

so this needs to be done for every tag.

ghost of drupal past → created an issue.

nicxvan → credited ghost of drupal past → .

The clean, total solution to all problems current and future is to have a class extending PHPStan\PhpDocParser\Printer\Printer much like Drupal Pretty Printer extends the php-parser printer which inspired the phpstan phpdoc printer. This printer can put in the HTML tags which can be turned into links much like it already happens with the code. Easy.

This is now for review. I dropped php-parser v4 support from Drupal Pretty Printer , there's no need for that now.

The issues were really minor:

1. ParserFactory::create is gone.
2. Expr_ArrayItem is now ArrayItem
3. A few things got deprecated but had 100% equivalent replacements so I did that.

Drupal Pretty Printer (which was api not long ago) had two minor problems too, one of them the same as above:

1. In method p for the Cast case it didn't pass on the arguments new to v5. The method did receive those so it was just adding these to the pPrefixOp call.
2. pExpr_ArrayItem needed to be come ArrayItem. By moving the logic into a common method it would've been possible to keep v4 support but I am not really interested in that any more.

ghost of drupal past → made their first commit to this issue’s fork.

ghost of drupal past → made their first commit to this issue’s fork.

Please check the overrides already in core/lib/Drupal/Component/Transliteration/data for example eo.php or kg.php and provide a similar ja.php. The file is a simple PHP array where the keys are Unicode code points and the values are transliterated strings.

Strong agreement: the Hungarian Drupal Slack was already consuming the article "Object-Oriented Form API in Drupal 11" before I got there to stop it. I can tell you with authority that article has nothing to do with what's new in 11.x for form/render API. Absolutely none. Part of it is probably lifted from chi's fundamental comment in the "Use the builder pattern to make it easier to create render arrays" issue which indeed sparked the current work but at the end it is not the developer experience we have chosen for all sorts of reasons. Other parts, I have no idea, quite likely it's some sort of AI hallucination from other parts of core. I tried to reach out to correct the article but was not successful.

larowlan and I wrote change notices which should be good guidance and provide an easy comparison of what's real vs what's this article.

1. X is obviously done for and shouldn't be used
2. Mastodon is what happens when the web people look at all the opportunities missed by Linux and wholeheartedly agree to create a platform downright hostile to new users and unusable to everyone else due to missing search because ideology trumps usability every time. Congrats. (For context, I pay for my own Mastodon instance. The point still stands.)
3. BlueSky is what happens when people say "see how Twitter worked out? Pinky swear it won't this time because (reasons which are not quite the whole truth to say gently)".

Anyone has a better timeline? This one sucks.

As far as I can see this is no longer an issue. Please comment which element is missing, if any.

The IS links the file which didn't change, it didn't need to change but the class at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Field%21P... now shows Attributes.

If I weren't paid to fix this I would kick this back to the core queue saying stop writing documentation for phpcs and start writing documentation for developers but as is, I will of course fix it but I couldn't resist noting this.

Note for the archives, to see what is in the members table, one can take the query from the EmptyClassTest and log like this:

    file_put_contents('/tmp/before', print_r(\Drupal::database()->query('        SELECT member.object_name AS member, class.namespaced_name AS class
        FROM {api_branch_docblock_class_member} dcm
        INNER JOIN {api_branch_docblock} member ON dcm.docblock = member.id
        INNER JOIN {api_branch_docblock} class ON dcm.class_docblock = class.id
        ORDER BY class, member')->fetchAll(\PDO::FETCH_ASSOC), TRUE));

and change the log file to /tmp/after in the version you want to see the changes in and then diff -u /tmp/before /tmp/after|grep -v Array shows crystal clear what are the changed members.

To sum up a very hard and long debug session: On DocFile::delete needs to remove all the docblocks for the file being deleted but this creates another bug because DocBlock presumed DocFile::delete doesn't clean up so that delete call needed to be removed.

ghost of drupal past → made their first commit to this issue’s fork.

ghost of drupal past → made their first commit to this issue’s fork.

That didn't work out quite right but I think 📌 Form element document for 10.3.x does not include any elements Active solves this one too.

📌 Form element document for 10.3.x does not include any elements Active subsumed more than half of this issue. One link remains to be solved. Once that's in I will rewrite the IS.

ghost of drupal past → made their first commit to this issue’s fork.

Sorry , wrong branch after commit. It's a different issue. Nevermind it.

my brother, you’re not helping these issues with your drama.

And derailing the issue is? We were near agremeent.

I tried. xjm returned and killed the issue because she hates me, sorry it's a fact, it's been six years and even that's not enough.

We might as well say that everything should just be marked public

The fundamental difference is how protected properties can have logic hardwired to them. Once Drupal core requires PHP 8.4 and property hooks become available this is a proposal I would wholeheartedly support, however.

Regarding final

the actual language feature, which is useful

Useful for what? For example: it explicitly tells the developer "this is not intended for extension". -- yes, @final does exactly that.

Ah but that was the listing page and the bug report is about the object page. I have updated Formatter::pageFunction to use the override documentation for method, param and return documentation.

This is unusual: there's testing ensuring this doesn't happen, in ComplexItemPagesTest:

    $this->assertSession()->linkExists('SubInSubDirSample::bar', 0, 'Overridden function link is there');
    $this->assertSession()->responseContains('A public method');
// the method in question:
class SubInSubDirSample extends SampleInSubDir implements SampleInterfaceTwo {
  /**
   * {@inheritdoc}
   */
  public function bar() {
  }
// the interface in question
interface SampleInterfaceTwo {
  /**
   * A public method.
   */
  public function bar();
}

I will investigate what breaks in the core case.

ghost of drupal past → made their first commit to this issue’s fork.

ghost of drupal past → made their first commit to this issue’s fork.

ghost of drupal past → made their first commit to this issue’s fork.

ghost of drupal past → made their first commit to this issue’s fork.

I already reported the AI slop as spam, you didn't need to act on it. This change has been reverted.

ghost of drupal past → made their first commit to this issue’s fork.