We might need to update this method in LocalReadOnlyStream as well.
Do we really need to check the operation ourselves?
Well, that actually was already noted many years ago.
https://www.drupal.org/project/drupal/issues/1308054#comment-6772280 →
Facing same issue when trying to acquire non-blocking lock.
Change unrecognized operations in LocalStream::stream_lock() to return FALSE;
Do we really need to check the operation ourselves? flock will return the correct error message if the operation is not permitted.
> \flock(\fopen('/tmp/test', 'wb'), LOCK_NB);
ValueError flock(): Argument #2 ($operation) must be one of LOCK_SH, LOCK_EX, or LOCK_UN.
Re: #18
This snippet works for me.
try {
$lock_exists = (bool) $this->database
->select('semaphore')
->condition('name', $name)
->countQuery()
->execute()
->fetchField();
if (!$lock_exists) {
$this->database->insert('semaphore')
->fields([
'name' => $name,
'value' => $this->getLockId(),
'expire' => $expire,
])
->execute();
}
}
finally {
unset($transaction);
}
Overall, I think the default lock implementation should use flock backend instead of database.
This needs to be fixed in 4.x branch first.
@niklan That patch prints absolute paths to the HTML files, not URLs.
I got lots of test failures with Chrome 134.
Ref:
https://issues.chromium.org/issues/405607581
https://github.com/teamcapybara/capybara/issues/2800
#58 works well on Drupal 11.1.4
I think it needs a more generic follow-up not focused only on Symfony Form component. Something like "Modernize Drupal Form API".
Right. That note was mostly for custom projects that override cache backends like follows.
$settings['cache']['bins']['bootstrap'] = 'cache.backend.apcu';
$settings['cache']['bins']['config'] = 'cache.backend.apcu';
$settings['cache']['bins']['container'] = 'cache.backend.apcu';
$settings['cache']['bins']['discovery'] = 'cache.backend.apcu';
$settings['cache']['bins']['menu'] = 'cache.backend.apcu';
$settings['cache']['bins']['toolbar'] = 'cache.backend.apcu';
It worth noting that prewarming data cached in APCu backend does not make mush sense.
chi → created an issue.
There is also an issue with EntityStorageInterface
https://github.com/mglaman/phpstan-drupal/pull/827
The patch may cause BC break on existing sites. However, I think it is acceptable in RC.
Actually #3 is wrong. The snippet in issue summary looks correct.
As of feature request I don't think it worth implementing. It looks more clear when checking for emptiness expensively. Function like drupal_view_if_not_empty() would look wierd.
It looks good. If the problem is only about rendering empty HTML markup you can also consider using CSS has selector to hide it.
but even when the View is empty, it contains empty HTML elements
That's a known issue in Drupal theming system.
Re #20. Turns out all implementations in Drupal core use void instead of null. Given, can only be used as a standalone type we have to update them. Otherwise we will never be able to add native return type hint to this hook. I agree, that needs to be done in a follow-up.
It doesn't return anything if the if condition is not met (void).
Right. Needs to be fixed.
I think we should add a :?array|int
Union types and the nullable type notation cannot be mixed. Anyway, as this issue is about @return tag I propose we don't add the typehint here. This is managed in other issue. 📌 [META] Add return types to hook implementations Active .
Re #9.
The documentation for the @return tag consists of two parts: the type hint and a free-form description. I believe the issue is that they contradict each other.
I wonder if we can explicitly document -1 in return type. PHPStan allows specifying scalar values as types in PHPDocs.
https://phpstan.org/writing-php-code/phpdoc-types#literals-and-constants
This is still relevant.
Can you clarify why? The issue summary points to Drupal 7 lock implementation. If it's still relevant can you describe remaining tasks that need to be done?
Possible solution is execution SELECT query before the INSERT in the same transaction. Though it may behave differently depending on configured transaction isolation level.
We can also consider using some third party lock implementation, for instance symfony/lock.
Re #7 Writing a test requires concurrent requests and reading PostgreSQL log. That's not what Drupal testing system is capable of. To reproduce the issue manually you can run simultaneously a couple CLI scripts which acquire a lock with the same ID.
The root reason of this error is explained well in #14.
Is this duplicate of 📌 Add typehints to hook definitions Active ?
Wonder if we could go with LFU cache instead. Will the implementation be more complex?
PHPStan currently does not have a rule for checking less specific return types. Psalm does.
I'v just created a feature request for PHPStan project.
https://github.com/phpstan/phpstan/issues/12442
I think that because Boolean is the data type, not false or true. https://www.php.net/manual/en/language.types.php.
@quietone true and false are covered there in value types section.
Re #36. That's correct. And if we should probably emit some notice when assertions are in production mode when running tests.
The logic for loading default image is in the image formatter. You may invoke it like follows node.field_image|view.
However, if you only need a raw image URI you have to use preprocess hook to fetch default image from field storage settings.
I also concerned about how to find the instances that this is suggesting be changed. A simple grep will not work.
Instances found with grep needs to be filtered manually by comment referring to false.
Like this.
The new temporary filename, or FALSE on failure.
I checked core code. Found 162 occurrences of false in PHPDoc tags. FALSE is not used at all.
@quietone Why do you think this violates Drupal coding standards?
* @return string|false
* The new temporary filename, or FALSE on failure.Well, there are a few other cases like this. I suppose the issue needs to be expanded to fix all of them.
Asking Composer runtime API every page load is potentially a performance hit, but it's simple to have Composer write a file containing the data we need during Composer installation.
Composer stores locations of every package including Drupal core into vendor/composer/installed.php file. Given that is a PHP file no performance hit should be expected (thanks to Opcache).
If that's still the case, then we can't rely on Composer being present.
We don't need Composer itself. InstalledVersions class is included to the tarball.
this needs a thorough investigation of the performance cost.
I did it in #26. Would be nice if someone else tested it to confirm.
I guess it happens because Claro is using content property for drawing the separators. The possible fix would be to use background-image instead like Olivero does.
@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 📌 Allow omitting doxygen when type and variable name is enough Active . 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.