@nicxvan we probably need to understand why "Implements hook_*" comment was initially added to Drupal coding standars. I guess it is needed to distinguish ordinary PHP functions from functions that implement Drupal hooks. Obviously with the Hook attribute is no longer needed. Also api.drupal.org might use this notation when listing hook implementatations.
For me it seems that in most cases docblock in hooks is not needed at all.
This issue is very similar to #3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough → . Maybe we should take same approach once it is resolved.
Re: #292 drush gen phpstorm-meta will be able to generate these completions in the next release.
Cross posted from 📌 OOP hooks using event dispatcher Needs review . The "implements" seems redundant now.
/**
* Implements hook_help().
*/
#[Hook('help')]
Do you think that "implements" note is redundant for this kind of hooks?
It gets worse in single hook classes.
/**
* Implements hook_help().
*/
#[Hook('help')]
final class Help {
/**
* Implements hook_help().
*/
public function __invoke(): void {
}
}
Certain hook groups should be in a class together.
I would force using single class hooks wherever it is possible. There very few use cases when hooks are highly related to each other.
/**
* Implements hook_help().
*/
#[Hook('help')]
Do you think that "implements" notes are redundant for this kind of hooks?
The fix seems trivial.
Note that @inheritdoc in \Drupal\entity\Access\EntityRevisionRouteAccessChecker::access is not actually referencing anything.
Drupal currently does not support image styles for files stored in extension directories.
That is the time format that all datetime fields expect.
Because it's storage format. It is rarely used in UI.
You may make the method more flexible by adding optional format parameter like follows.
function getNow(string $format = DateTimeItemInterface::DATETIME_STORAGE_FORMAT)
Is that a necessity?
It's a general reason for having services injected. Core uses @datetime service when needs to obtain current time.
Hopefully, someday it'll switch from the custom baked TimeInterface to PSR-20.
I am thinking of making all TT services final to avoid such issues in feature.
Changing constructor signature could break existing sites that extends these services. Though the should not be considered as public API.
Anyway, I propose we change this the next major release. I've just created 4.x branch.
Why are the cases coded in camel case? They are like constants. Shouldn't we use uppercase for them?
I've just released a module that implements the date/time fields in a different way.
It also supports HTML5 elements in views exposed forms
https://www.drupal.org/project/date_point →
There are a few concerns with the proposed approach.
1. The implementation is hardcoded to one specific date format (Y-m-d\TH:i:s).
2. It overlaps with PSR 20 where now() is supposed to return DateTimeImmutable object.
3. It does not use core time service. So no way to mock the date in tests.
Meanwhile, Date Point's build is green again. Has something been already changed?
https://git.drupalcode.org/project/date_point/-/pipelines/359856
I like the idea of "Golden Contrib" which was proposed years ago
https://www.drupal.org/project/drupal/issues/1255674#comment-4917546 →
It's basically a curated list of Drupal modules and themes.
For each project in this list DA must ensure that
- It has active maintainer
- It has a stable release compatible with the current Drupal core
- It has proper documentation
- It complies with the strategy of Drupal CMS
DomDocument is still pretty verbose.
DomDocument was referenced mainly as an example. I don't think it's the right tool for this. See note #6.
I was looking for something Yii HTML but with support of attachments and custom render callbacks.
I think such a big change deserves a change record and 11.1 release highlights.
What was the reason that 'label_count' is moved away from other 'label_*' parameters?
All we need is a mechanism for the reset, that does not pollute the public interface.
If we were using the Symfony core, we would already have such a mechanism.
Anyway, I suppose it's not a big deal to enable this service pass in Drupal kernel.
\Symfony\Component\HttpKernel\DependencyInjection\ResettableServicePass
unless they use some kind of magic plugin that reads the services.yml files
For PhpStorm you can generate meta information about services.
drush gen phpstorm-meta
Psalm also reads it. Not sure about PhpStan. I suppose phpstan-drupal can help on this.
#2602368: Allow objects that implement RenderableInterface to used interchangeably with render arrays. → indeed is very close to this one. Though the proposed approach is a bit different.
Here we aim to completely remove render arrays.
In #2602368 the render arrays will remain as intermediate layer between elements (objects) and renderer service. That is much easier to implement. The submitted patch is actually a tree-lines change.
As of BC, we don't have to implement it imminently. Custom projects can benefit from this change right now. Contributed projects may start using element objects in major releases where BC breaks are allowed.
I think this issue has wider scope than 🌱 [policy] Standardize how we implement in-memory caches Needs work . Application state is not only about static caches. There are some other resources that may need to be reset between requests. DB connections, temporary files, open streams, etc. In Spiral framework that process is called finalizing. Finalizers are responsible for cleaning up resources. For instance, logger finalizer will call reset method on Monolog loggers.
Besides resource management there is another challenge. Request aware services. In core.services.yml I counted 47 services that has direct dependency on request_stack and there are many services that has indirect dependency through services like @current_route_match, current_user, datetime.time, etc. In Spiral that implemented with "scoped containers". For each request it creates a scoped container derived from root container with some request specific services.
One more issue to address is stateful Drupal kernel. It has a few of boolean flags (booted, prepared, etc) that needs to be reset after each request.
It takes 0.015 ms on my localhost. That file is likely loaded from opcache and Composer caches the array in static variable. So it's Ok to call this method multiple times.
@oily The are dozens different render elements in Drupal core. The MR currently covers just a few ones.
A bit surprised how much this constant is used in Drupal.
Submitted a MR for plain text formatter.
I don't know how is JsonMarkup supposed to work here.
Turns out that default "Plain text" formatter has same bug.
Given that expressions are not allowed in class constants we will have to deprecate that constant in favor of static method.
public static function version():string {
return InstalledVersions::getPrettyVersion('drupal/core');
}
We may use getPrettyVersion() instead.
echo Composer\InstalledVersions::getVersion('drupal/core'), \PHP_EOL;
echo Composer\InstalledVersions::getPrettyVersion('drupal/core'), \PHP_EOL;
Output on 10.3.10
10.3.10.0
10.3.10
Output on 11.x
11.9999999.9999999.9999999-dev
11.x-dev
@oily that one was about setting Drupal version to 11.2-dev for 11.x branch. That's exactly what MR in this issue does. I am fine to close 🐛 Update Drupal::VERSION in dev branch Active as duplicate.
The relevant Slack conversation:
https://drupal.slack.com/archives/C1BMUQ9U6/p1729010405763549
Note that we will have to update that version to 11.3-dev once 11.3.x branch is open for development.
Re #10
By "documentation," I mean a list of available properties with their types and a brief description. This documentation may already exist somewhere, but you would need to look it up yourself. For objects with typed properties and methods, IDE can provide that information for you. See ✨ Use the builder pattern to make it easier to create render arrays Needs work for details.
How do we handle BC, especially with alter hooks that expect to find arrays?
I've no idea at this moment. Another hook? Anyway, this needs some brainstorming.
Isn't this a duplicate of 🐛 Update Drupal::VERSION in dev branch Active ?
Re #8 That make sense.
Technically, the Drupal already has sort of DOM based on render arrays (should we call it DAM?). The issue merely about replacing associative arrays with objects.
it's better to create a lightweight element tree with a few helper methods to simplify manipulation and traversal
nicmart/tree seems like the right tool for this, at first glance.
Re #5
That requires benchmarks. My guess is that manipulating an object tree costs almost nothing compared to other operations performed by Drupal's theme layer, such as preprocessing variables and rendering Twig templates.
Actually, I think, having a full-fledged DOM implementation is not needed. We only need DOM as backing store for content tree. From this point, it's better to create a lightweight element tree with a few helper methods to simplify manipulation and traversal.
PHP’s built-in DOM extension feels overly complex for such tasks and has some pitfalls when dealing with multiple DOMDocument objects. On the other hand, it offers some cool features that could be nice to have, such as traversing in the upward direction, XPath, and CSS selectors (introduced in PHP 8.4) for finding elements. However, none of these features are essential for Drupal's rendering system.
Overall, this will only mitigate the problem. Dropping render arrays is the only way to fully resolve the issue. I’ve filed a ticket in the ideas queue.
Meanwhile, we can use a render element to gradually migrate from render arrays to DOM.
/**
* Provides a render element to build markup using document object model.
*
* Properties:
* - #dom: Document Object Model
*
* Usage Example:
* @code
* $build['dom'] = [
* '#type' => 'dom',
* '#dom' => $dom,
* ];
* @endcode
*
* @RenderElement("dom")
*
* @see https://www.php.net/manual/en/book.dom.php
*/
final class Dom extends RenderElementBase {
/**
* {@inheritdoc}
*/
public function getInfo(): array {
return [
'#pre_render' => $this->preRenderEntityElement(...),
'#dom' => NULL,
];
}
/**
* Pre-render callback.
*/
public static function preRenderEntityElement(array $element): array {
$dom = $element['#dom'] ?? NULL;
if (!$dom instanceof \DOMDocument) {
throw new \InvalidArgumentException('#dom property must contain instance of DOMDocument');
}
// Instead of using saveHtml, it should traverse the tree recursively,
// rendering child nodes while respecting theme hooks and bubbling attached
// metadata.
return ['#markup' => $dom->saveHTML()];
}
}
PHP has evolved a lot since this issue was created. Type hints, property promotion, named arguments, etc. So instead of builders we can create value objects. It'll give same DX in modern IDEs.
Builder
$variables['table'] = TableElement::create()
->setHeader($headers)
->setRows($rows)
->setAttributes(['id' => $table_id])
->setTableDrag([
[
'action' => 'order',
'relationship' => 'sibling',
'group' => $weight_class,
],
],
)->toRenderArray();
Constructor
$variables['table'] = new TableElement(
header: $headers,
rows: $rows,
attributes: ['id' => $table_id],
tableDrag: [
[
'action' => 'order',
'relationship' => 'sibling',
'group' => $weight_class,
],
],
)->toRenderArray();
We can't require write permissions to a PHP file in production.
For Twig files it was resolved in #2544932: Twig should not rely on loading php from shared file system → . Can it be done same way for containers?
Anyway DB based container cache may still exist and used as fallback for sites that need container rebuild in non CLI SAPI. The point is to use more efficient solution by default.
implies opening DB connection
Opening DB connection is rather expensive operation, especially in PostgreSQL. Even if the cache is managed through in memory backend (Redis/Memcache) Drupal will open DB connection as bootstrap container depends on Database service.
Re #99. That makes sense. Static cache is the fastest cache ever. It would be not wise to throw it away. I think the potential performance gain from static cache might be comparable with the one from skipping bootstrap phase. The application needs to have a control on what needs to be reset.
Wonder if it's better to create a new cache backend.
Relevant conversation: https://www.reddit.com/r/PHP/comments/vlyw1g/benchmarking_serialization/
I did some benchmarking locally. Getting cached items back is almost instance when using VarExporter.
Here are some results of profiling Drupal site with cache.container implemented with PhpBackend
Here are the results:
For local Drupal 11 installation the performance boost was about 5-10% for anonymous visitors.
I didn't expect that someone still uses it.
Still trying to comprehend how this event loop will work with new MySQL driver (
📌
[PP-1] Create the database driver for MySQLi
Postponed
). As I understand the revolt/event-loop is based on streams. stream_select is essentially a backbone of its async operations. That means the DB driver should be implemented through PHP streams like amphp/mysql.
Did I miss something?
This would all happen in the non-async database connection
I meant a single HTTP request without any concurrency. In that case in non-async database all queries will happen sequentially. So no locks expected.
I created a module to test options described in #20. It builds sort of landing page with blocks that can be slow down.
https://github.com/Chi-teck/sample_catalog
Results are quite interesting though predictable.
When blocks are too slow it doesn't matter which way you are doing parallel processing. The results are always quite good. I've managed to get 12x boost using 12 CPU cores. However, when blocks are relatively fast, the cost of spawning new processes becomes significant. In that case the best results were achieved with "co-pilot" server powered by Road Runner. It demonstrated its usefulness even when building each block takes about 5-10 ms.
Overall, landing pages are not the only use case for this. For instance, I have a project with very heavy API endpoint for a collection of entities. Each item in that collection is personalized and is frequently updated. So caching is not possible. Building items in parallel using one of the above mentioned options can potentially improve API performance a big deal.
Re 18: There are a couple things that can potentially break that calculation.
1. Database locks
When cache is empty the request will likely trigger lots of cache updates that potentially may case raise conditions. Especially when same cache items need to be updated in different blocks.
2. Building vs rendering
Those terms are often misused. Building means creating a render array while rendering is creating an HTML presentation of the content (string or Markup object). The problem here is that blocks typically relies on lazy builders, pre_render callbacks, theme functions etc. That stuff delegates rendering process to theme layer.
Consider this render array. It costs nothing for CPU to produce such content. The main work will happen later when Drupal is rendering content. And that means that async orchestration have to cover theming as well.
$build['content'] = [
'#theme' => 'example,
]
Any way for sites where SQL queries is not a bottleneck the only solution would be having additional PHP processes.
Could think of a few options here:
1. Spawning new processes with proc_open (Symfony Process). For example, for landing pages you described about those could be very simple PHP scripts that do just one thing - render a single block.
2. Rendering blocks with PHP-FPM through hollodotme/fast-cgi-client. That should be faster than CLI workers as it compatible with Opcache and APCu. And supports async requests as well. Also FPM has some options to control number of workers in the pull.
3. Having a few workers connected to a queue. They should listen for jobs (block rendering) and reply instantly. That means Drupal DB based queue is not quite suitable as it would require frequent polling.
4. The most extreme option. Having a multi-thread socket server written in PHP with Drupal bootstrapped. It should be able to handle multiple connections simultaneously. So when a Drupal site needs to render a dozen heavy blocks it just send tasks to that sever to render blocks in parallel.
I suppose nothing of that can be fully implemented in Drupal core. However it could provide some async API to allow delegation of block rendering to external systems.
Re: #18. We can start rendering blocks with fast queries while waiting for query results from blocks with slow queries? Is it correct?
Additionally, other CPU intensive tasks (like rendering results) could be happening while waiting for all three queries to come back.
That's unclear. How do we render results without having those result yet?
Drupal is both i/o and CPU bound, but we can do both database query execution and CPU-intensive tasks in parallel once we have async database queries implemented.
Having async DB driver in place we can run queries in parallel. But is it really possible for CPU tasks like rendering?
@catch Those number are not for measuring site performance. The point was to check how many IO bottlenecks we potentially have. And again checking it in CLI SAPI is not correct.
As of benchmarking, testing just front page may not be sufficient. I think Drupal needs a comprehensive set of load tests that can help to figure out performance gains and track performance regressions.
I created a few K6 scenarios to for testing performance of Drupal site. Wonder if Drupal core could implement something similar as part of CI workflow.
https://github.com/Chi-teck/k6-umami
Re #13. It was custom project with a few millions entities. However the front page is just a user login form without extra blocks.
Here are results with brand new D11 installation.
Cold cache
real 0m0.638s
user 0m0.346s
sys 0m0.103s
Warned cache
$ time php index.php > /dev/null
real 0m0.123s
user 0m0.086s
sys 0m0.030s
Though standard profile does render many things on front page.
Yes, there are lots of IO operation
I guess those were mostly file operations performed by Composer autoloader, because in CLI Opcache and Apcu do not make much sense. In real HTTP request served by FPM the difference between real and user will be much smaller.
That's not clear how will Drupal benefit from this.
This command can give a very rough estimation of how much request time we could potentially save. Compare real and user time.
$ time php index.php > /dev/null
real 0m0.242s
user 0m0.152s
sys 0m0.055s
Yes, there are lots of IO operations but not many of them can be taken out of main code flow because subsequent steps typically depend on previous ones. I believe the earlier mentioned cases (Cache Prewarm and Big Pipe) won't give big performance gain. They need benchmarks to prove their usefulness.
Re #18. It happens without this MR as well. Right?
a separate Twig extension
An example of such extension.
https://github.com/jawira/case-converter-twig
Integration it into Drupal should be as simple as declaring a service in yml file.
There is a couple reason why this shouldn't be part of Twig Tweak.
1. Twig Tweak does not have to incorporate all possible Twig functions and filters. There is no such a goal to become the only Twig related module in Drupal. Specific use cases should be addressed in specific modules. Twig casings covers quite specific use case. Btw, I did similar filters in Drupal Code Generator.
2. Twig Tweak does not aim to improve Twig itself. It's primary focused on integration with Drupal. Сheck out list of our functions and filters. They are almost all related to Drupal. I think general things like string transformation should be implemented in Twig core or in a separate Twig extension (ordinary Composer package, not a Drupal module).
Thanks.
I wonder if $entity->toUrl()->access() is more appropriate here than $entity->access('view').
I actually think all cache backends should use currentTime.
chi → changed the visibility of the branch 3483370-drupalcorecachememorybackend-should-not to hidden.
chi → created an issue.
alexpott → credited chi → .
Once core is converted, it will be possible for a site to run without any procedural hook implementations at all.
This requires that none of the installed contributed modules implement procedural hooks. Right?
@nicxvan yes.
Stack-trace looks like follows
(
[0] => Array
(
[file] => /web/core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php
[line] => 332
[function] => parse
[class] => Drupal\Component\Annotation\Doctrine\StaticReflectionParser
[type] => ->
)
[1] => Array
(
[file] => /web/core/lib/Drupal/Core/Hook/HookCollectorPass.php
[line] => 164
[function] => getMethodAttributes
[class] => Drupal\Component\Annotation\Doctrine\StaticReflectionParser
[type] => ->
)
[2] => Array
(
[file] => /web/core/lib/Drupal/Core/Hook/HookCollectorPass.php
[line] => 122
[function] => collectModuleHookImplementations
[class] => Drupal\Core\Hook\HookCollectorPass
[type] => ->
)
[3] => Array
(
[file] => /web/core/lib/Drupal/Core/Hook/HookCollectorPass.php
[line] => 74
[function] => collectAllHookImplementations
[class] => Drupal\Core\Hook\HookCollectorPass
[type] => ::
)
This somehow broke tests on a custom module. Cannot reproduce it locally, but CI output is full of such errors.
Notice: file_get_contents(): Read of 12288 bytes failed with errno=21 Is a directory in web/core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php on line 184
After some research I found a contributed module that faced same issue.
🐛
phpunit (next minor) failing
Active
Right. It needs update hook.
probably because that output was not in PHPUnit 9
In PHPUnit 9 that line is printed only when --verbose option is provided.
I wonder if we really need to update the version manually. Can we fetch the version from Composer instead?
Composer\InstalledVersions::getVersion('drupal/core')
@socialnicheguru this issue is about storing keys in settings not state.
Could not reproduce.
This might be somehow related to your site timezone.
Is there at least one contributed project that ships with its own phpinit.xml file?
Maybe somehow the phpunit.xml config is not being loaded?
It's certainly not loaded.
Confirm the date value was updated to tomorrow in the display and the edit form.
Did you check the date value in the database? I wonder if it's only related to formatter.
I just created one more generator for preload script.
https://www.drupal.org/project/opc →
However, while profiling my project I found another way to optimize opcache.
There are a couple opcache settings which default values are not suitable for production.
opcache.validate_timestamps=1
opcache.revalidate_freq=2
A typical Drupal sites loads about 1k - 2k files per request. With above configuration it'll revalidate opcache every 2 seconds.
Turning off opcache.validate_timestamps gave me almost same speed boost as preloading. I think, as long as you don't edit code on production that setting can be disabled. The invalidation should happen through php-fpm reload on deployment.
For those who might be stucked
The currently used constraint should not block you from using twig/twig v3.14.0
https://semver.madewithlove.com/?package=twig%2Ftwig&constraint=%5E3.10.3