xjm → credited alexpott → .
Fixed the merge conflict with 11.x
xjm → credited alexpott → .
alexpott → created an issue.
alexpott → created an issue.
phenaproxima → credited alexpott → .
@acbramley I've responded to your points on the new MR.
So I think we only need the StatisticsLastUpdated field, filter and sort to entity types which implement EntityChangedInterface and have a changed field. That way we avoid quite a few issues. If you want to filter on the last_updated time in comment_entity_statistics then you can use the last comment timestamp field, filter and sort... you do not need this special implementation.
@pandepoulus yep assigning the return $this->query->orderBy() to a property is pointless. I've updated the MR to fix this for 11.x.
We still need to add test coverage for the comments on entity types that do not implement EntityChangedInterface
alexpott → made their first commit to this issue’s fork.
This is really a duplicate of 🐛 Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column Needs review - let's put our efforts there to fix all of this there.
I think we should just remove the property. \Drupal\comment\Plugin\views\sort\StatisticsLastUpdated::$field_alias is unused and $this->query->addOrderBy() can only ever return a NULL. I think this was just a copy and paste fail.
alexpott → made their first commit to this issue’s fork.
We're not testing the change here. We need to test when there is a schema and you set ->save(TRUE) so the data is trusted.
Committed and pushed 01eecd5ceef to 11.x and 0cb41093905 to 11.2.x. Thanks!
Backported to 11.2.x as a test only change.
I think we have a few things to address.
I don't think we should be changing the string ones here. It's pointless and I think should be address in the issue that does the change.
Maybe there is a downside - maybe we need to ensure that node_access is rebuilt on module uninstall when there are no node grant implementations - or when there was and with the uninstall there is not.
alexpott → created an issue.
One thing that is "interesting" is that node_access_view_all_nodes() is documented to return a bool. ATM in 11.x it only does this if there are no hook_node_grants implementations. Otherwise it returns a 0 or 1 integer because that's what Drupal::entityTypeManager()->getAccessControlHandler('node')->checkAllGrants($account) returns. With the new MR it will always return a 0 or 1. Boo to us and our lax return types. I think we should do a separate issue and CR to change \Drupal\node\NodeAccessControlHandlerInterface::checkAllGrants() and \Drupal\node\NodeGrantDatabaseStorageInterface::checkAll() to return a bool as this is a TRUE / FALSE access check and an integer is confusing.
So after refactoring node_access_view_all_nodes() into \Drupal\node\NodeGrantDatabaseStorage::checkAll() - see https://git.drupalcode.org/project/drupal/-/merge_requests/12586 - I think this issue can return to it's original intent - because there is no real API change. And then we can use
📌
Deprecate node_access_grants() and move its functionality to NodeAccessControlHandler
Active
to decide how to deal with node_access_grants() as part of an issue that is completely API focussed.
@acbramley sorry it is taken a while to see through the API trees here but I think that the new MR points to a better place :)
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
We need to do some work on the support of regular expression strings.
1. The form help text does not seem right
2. We need to validate the strings
3. The defaults are too greedy
4. The update function does not look right.
Backported to 11.2.x to keep test coverage aligned and this is a test-only change.
Committed and pushed c889311f82f to 11.x and 5176cb2d3e8 to 11.2.x. Thanks!
I moved the test to be alongside where the exception is tested and I improved the test to check for more things.
Committed and pushed 8cd685b9f10 to 11.x and 86a448df9c8 to 11.2.x and 2f93f58bd14 to 10.6.x and fc389ed0fc6 to 10.5.x. Thanks!
Backporting to 10.5.x as a bugfix and I've run the tests there and everything is fine.
Updated issue summary to explain why removing MySource.Debug.DebugCode with no replacement is fine - considering this was not explained on the issue.
Oh I'm not sure that MySource.Debug.DebugCode was actually doing anything useful... debug is not a PHP function - maybe it was protecting against usage of some class called Debug - but Drupal has never had one of those. Ignore this.
alexpott → created an issue.
alexpott → made their first commit to this issue’s fork.
Committed and pushed 2ffd3412fe7 to 11.x and 4842c65b535 to 11.2.x and ca127551d4f to 10.6.x and 642eb99e04e to 10.5.x. Thanks!
Backported to 10.5.x as this is non-runtime code and it does not hurt to have everything in sync.
@acbramley I think we should widen the scope to include the other function here. Doing it piecemeal results in a less complete API for no real gain.
@acbramley yes I think this issue could create a service for both of these methods in node.module. I've been trying to think if any of the existing node access services could be locations and I think I agree with the earlier posts - a separate service is a good idea. I considered node.grant_storage but I think this is not about storage and the new service needs to be injected into that one. I also considered cache_context.user.node_grants - or maybe making a new service that is also a cache context but that felt overly complex to. Therefore a new service with two public methods for each function we're replacing and marked @internal because it is not meant to be extended - the extension points are the node grant hooks.
Also, the browser is already converting "\n" or "\r" to "\r\n" on submit, so technically for users on non-Windows clients, the input they entered has already been changed.
Really? Yes it does.... I've confirmed that with a debugger. This is quite messed up right? What a PITA. FWIW though how real is it to enforce max length on the database level for a field using a text area? And is doing that even correct. I don't think it is. Sure you can enforce max length on the field level but backing a text area field with anything other than big text field feels like you are opening a can of worms.
Also I think that point 4 from #27 is important.
Adding issue credit.
As per @acbramley, I'm concerned that the focus of this issue is about the use of drupal_static and not the node access API. I think it is great to refactor out usages of drupal static - but this needs to be done with an eye to the resulting API. We have the following node access related functions left in node.module and we should have a plan for them:
- node_access_grants
- node_access_view_all_nodes
- node_access_needs_rebuild
- node_access_rebuild
- _node_access_rebuild_batch_operation
- _node_access_rebuild_batch_finished
Are there issues for the other methods? It feels as though they split down into two groups:
- node_access_grants and node_access_view_all_nodes
- The *rebuild* ones
Are there existing issues? I think not looking at 🌱 [meta] Deprecate node.module functions Active .
I think we need to agree the API shape before doing this issue and once we've done that we should focus the issues on doing that and as a by product remove drupal_static usage where we can.
I think that converting node_access_grants and node_access_view_all_nodes to a single service makes sense. I also think the service should be marked as @internal because it is API glue and if you want to affect this API you sure be implementing the hooks and not doing anything to this service.
I'm really not sure what to do here.
On one hand it is already easy for a recipe to set the behaviour it needs. On the other, the reason this strictness exists is to give recipes a solid expected ground to work on. @berdir is obviously correct that the easy examples like the language one given where strictness just gets in the way.
I feel that if we default to lenient then no one is ever going to benefit from strict and if don't change the default the idea of inter-operable recipes than many users can benefit from might not happen because users will hit this all the time. Especially will any recipe that does multilingual.
I guess that we need to go back to the solid ground idea and why that existed. I think there are two reasons; firstly the idea a recipe is idempotent - you apply it and you get the same results and secondly when you start applying config actions on top of configuration they are often making assumptions about the starting state of that configuration. The first idea needs challenging a bit because if a recipe is applied to a site and language has already been installed and configured and the recipe is not changing language configuration then the fact that the language config no longer matches the default state does not matter at all. The second idea is sometimes correct but not always and it feels as though the solution we have is too strict to solve this particular case. I think we need to look into better solutions for this and we already have issues open for this: ✨ Recipes have no way to tell if they're set up for success Active and #3463641: Create a way for recipes to check their preconditions → . I think if we land the precondition issue then we can change this default - and perhaps consider deprecating the functionality and telling people to add preconditions instead.
@berdir what do you think about this idea? Does it work for you?
larowlan → credited alexpott → .
Some thoughts while reviewing this issue.
- Is it really okay to change user input. Normally Drupal saves what the user enters and makes changes on output. This is pretty much how the filter system works. We do have precedence for changing the value on input TextField does
return str_replace(["\r", "\n"], '', $input);. - Should we make the change to \Drupal\Core\Form\FormValidator::performRequiredValidation so any contrib or custom implementations benefit given that that is where max length validation occurs.
- What about \Drupal\Core\Validation\Plugin\Validation\Constraint\LengthConstraint and the constraints - looking at our text field types we are not using constraints - but we probably should at some point.
- This will make values submitted via forms behave different from JSON:API - which will be surprising.
I'm not sure what all these thoughts mean at this point but I just wanted to note them down.
xjm → credited alexpott → .
I've tried to write a test for this but it is hard - I can;t get composer to use an alias for our path repositories used in testing.
I've now got even less steps to cause the problem
composer create-project drupal/recommended-project:^11 recipe-test
cd recipe-test
composer require "drupal/inline_translation:1.x-dev as 1.0.0"
But doing $this->runComposer('require "fixtures/module-b:2.0.1 as 3.0.0"') in a \Drupal\Tests\Composer\Plugin\Unpack\Functional\UnpackRecipeTest does not cause the problem :(
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
xjm → credited alexpott → .
Steps to reproduce from @rkoller
composer create-project drupal/recommended-project:^11 recipe-test
composer require drupal/eca drupal/bpmn_io
composer require drupal/eca:3.0.x-dev drupal/bpmn_io:3.0.x-dev drupal/modeler_api:1.0.x-dev
On the second require you will see drupal/eca does not resolve to a package. in the output.
Can you add your actual steps to reproduce this issue. I cannot reproduce this. Here's the output of composer --version for me...
Composer version 2.7.7 2024-06-10 22:11:12
PHP version 8.3.21 (/opt/homebrew/Cellar/php@8.3/8.3.21/bin/php)
Here's what I'm doing:
composer create-project drupal/recommended-project:^11 recipe-test
composer require drupal/events:1.0.x-dev
composer require drupal/inline_translation:1.x-dev
composer require drupal/eca:3.0.x-dev drupal/modeler_api:1.0.x-dev
The events recipe is successfully unpacked and the modules are installed. No messages from the recipe plugin.
I've also updated to composer 2.7.7 and it does not happen for that version either.
A project I'm on has this patch applied - doesn't apply to the latest version - here is a patch that does...
Please let's not overwrite a file from core that would be the worst of all world. I really do not think there is an issue with vendor/drupal containing Drupal classes, not from an IDE or a conceptual basis.
Also with this change the root composer.json and a Drupal scaffold enabled site are running the same preAutoLoad dump script - which is a good thing because it'll improve the performance of sites because a site of classes that are required to make respond to a cache hit will now not be autoloaded they will just be included.
I rebased the MR to fix conflicts with 11.x
@rkoller the MR is saying we conflict with 1.1.0 so it will not be installed.
@divyansh.gupta thanks - the changes look good to me. I will leave rtbc to someone else so I can commit.
alexpott → made their first commit to this issue’s fork.
Given the above discussion with @Jordi Boggiano, I still feel that using vendor/drupal is the best place. I disagree that placing files in vendor is a Drupalism. I would argue that placing files outside of vendor is by far the bigger Drupalism. Code that is in vendor is much easier to place outside of the webroot and is therefore my secure. In an ideal world core would be in vendor and we would not be having this discussion.
Here's a transcript of my conversation with @Jordi Boggiano.
alexpott
@naderman @Jordi Boggiano I’ve got a composer plugin that needs to write a class during pre autoload dump. The contents of the class are based on information from composer. I’ve noticed that other composer plugins that do something similar write files to vendor/composer/ which feels really interesting to me and I don’t know why they do that. Is this the recommended place for plugins to write utility classes like this that depend on information that composer has?
Thanks for any pointers :slightly_smiling_face:
Jordi Boggiano
@alexpott hmm.. it is kinda the only reserved/safe place for composer to write things because we know for sure vendor/composer/autoload_real.php for example will never exist due to a package named composer/autoload_real.php as we control that vendor. Why people dump their stuff in there I do not know, can't say that I condone this but I assume they mostly do it because it feels like the place to dump random stuff because composer does it
Jordi Boggiano
it's fine as long as people use unique enough file names..
Jordi Boggiano
like GeneratedServiceProviderData and GeneratedDiscoveryStrategy are not strictly names I would consider namespaced/unique enough but hey, people take the chances they want to take
Jordi Boggiano
that's why plugins are great.. I don't need to maintain them if shit goes wrong:smile:
alexpott
@Jordi Boggiano thanks for reply. My own implementation dumped the file in my own namespace directory in vendor which feels safer. I was wondering why these other plugins did not take that approach and if there was a good reason because yeah the potential for file name clashes feels an odd risk to take.
Jordi Boggiano
yeah using your own vendor sounds safer to me
Let's added some test coverage of this behaviour. We could add this to \Drupal\KernelTests\Core\Config\ConfigCRUDTest::testDataTypes
alexpott → created an issue.
The problem is that the same date with different langcodes will not compare correctly. PHP's built \DateTime does compare properly and if it was the only property on our DateTimePlus objects then they would compare correctly too. But it's not so that don't and there is nothing we can do about this. I do think we should add a compare method to make things simpler...
Something like:
public function compare(DateTimePlus|\DateTime $datetime2, string $operator): bool {
if ($datetime2 instanceof DateTimePlus) {
$datetime2 = $datetime2->dateTimeObject;
}
return match ($operator) {
'<' => $this->dateTimeObject < $datetime2,
'<=' => $this->dateTimeObject <= $datetime2,
'>' => $this->dateTimeObject > $datetime2,
'>=' => $this->dateTimeObject >= $datetime2,
'==' => $this->dateTimeObject == $datetime2,
'!=' => $this->dateTimeObject != $datetime2,
};
}
One thing that will catch people out is that if the the dates have the same langcode then object comparison will work as a user expects and therefore I'd guess that we see a lot of date comparison in custom and contrib.
Committing only to 11.x as this is more of an improvement than a bug fix.
Seems fair enough to backport to 10.5.x.
Committed and pushed 76c2d39afb4 to 10.6.x and 8ee22c46ac4 to 10.5.x. Thanks!
I think this is a bug. Tests should not leave files around.
For some reason Symfony/Validator has not been updated to 7.3 - which is the minimum of the old class is still cached in OpCache - maybe restart your Webservers and check your opcache configuration as files should be refreshed if they change.
See https://github.com/symfony/symfony/blob/v7.3.0/src/Symfony/Component/Val... - the symfony class has the same typehint as Drupal.
We've definitely got test coverage of this update running on SQLite - see \Drupal\Tests\system\Functional\Update\RouteAliasUpdateTest - this test is running on SQLite. Also SMALLINT is only ever in Drupal's codebase in the mysql and postgres schema classes where we are converting from Drupal's data types to the dbs - the sqlite's equivalent is \Drupal\sqlite\Driver\Database\sqlite\Schema::getFieldTypeMap() and there is no mention of smallint. This report feels like Drupal is trying to use MySQL or PGSQL schema class with SQLite and that's definitely not going to work and I don't know how that could happen. I'd look in your settings.php and check that your database connection info is what you expect... I would also check what drush thinks the database type is using drush status.
Thanks for working on this and I agree we should make the change, however, I’ve discussed this with @catch and share his concerns about the impacts of changing the code to throw exceptions here. Yes the change is correct from a “programme to the interface” perspective but as we can see from core code callers are more often then not coded to work with the actual behaviour of the code. We need to give fair warning to custom and contrib that this change is coming. Therefore I think we should trigger an un-silenced E_USER_DEPRECATED to indicate that an exception will be thrown in Drupal 12 and we should file a follow-up to make the change in Drupal 12 and the changes to the callers.
This way modules can continue to work on 11 the way they do know and get ready for the changes in 12.x and it'll be easier to provide a continuous upgrade path. @cmlara I know that this process is likely to frustrate you - and understand that - it would be different if we had concrete cases where this is causing data loss or a security issue but we don;'t have that atm so we should prioritise the ability of sites to update without unexpected behaviour changes.
One thought is doing we really need to run all the nightwatch tests against Olivero and a starterkit theme - doing so is going to double our Nightwatch testing time as far as I can see. Maybe we only need to run the most generic of tests - or maybe we only need to run against a generated theme?
This is such a nice idea. +1
Added some comments to the MR - we shouldn't be adding return typehints in this MR.
Exciting work. I left a review on the MR.
alexpott → made their first commit to this issue’s fork.
Going to commit to 11.x only as I don;t see why po files should change in a patch release. It's not like this is causing any real bugs.
Committed be9783d and pushed to 11.x. Thanks!
There's so much odd code in this class...
foreach ($this->translation as $i => $trans) {
if (isset($this->translation[$i])) {
$output .= 'msgstr[' . $i . '] ' . $this->formatString($trans);
}
else {
$output .= 'msgstr[' . $i . '] ""' . "\n";
}
}
Afaics if (isset($this->translation[$i])) { must be true. I'm not even sure what that code is try to do... - maybe handle the case where one of the plural forms is not translated.
Anyhow this fix is fine.
Backporting to 11.2.x,10.6.x and 10.5.x as a bugfix.
Committed and pushed ec48de10e88 to 11.x and 0808066939c to 11.2.x and 3f31fab7c51 to 10.6.x and ff046030205 to 10.5.x. Thanks!
Whoops we cannot use rtrim here because of public:// - nice tests! Reverted my noise. Sorry.
Committed 0e61b8c and pushed to 11.x. Thanks!
Not backporting to 11.2.x because this is a string change.
Should this be backported to 10.6.x?
alexpott → made their first commit to this issue’s fork.
This is a nice find. I think the MR needs a few adjustments. See the MR review comments.
I also if we need to think about empty media queries in \Drupal\toolbar\Element\Toolbar::preRenderToolbar()
I don't think we should hardcode this. How about we add something to route so we have an API here that other modules could use.
Something like
if ($route_match->getRouteName() == 'entity.node.canonical' || $route_match->getRouteObject()->getOption('node.is_full_page_view')) {
And then in \Drupal\content_moderation\Entity\Routing\EntityModerationRouteProvider::getLatestVersionRoute() do ->setOption($entity_type_id . '.is_full_page_view', TRUE) on the route it returns. This way any entity that has similar needs to node can do things in the same way.
We should also update the docs in template_preprocess_node() as the following is not correct...
// The 'page' variable is set to TRUE in two occasions:
// - The view mode is 'full' and we are on the 'node.view' route.
// - The node is in preview and view mode is either 'full' or 'default'.
Let's not add the new API here. It's going to introduce subtle bugs that are worse than the deprecation being fix here.
Let's change the getMediaQuery() to
public function getMediaQuery() {
return trim((string) $this->pluginDefinition['mediaQuery']);
}
And the use in to
$media_query = $breakpoint->getMediaQuery();
if ($media_query !== '') {
$source_attributes->setAttribute('media', $media_query);
}
We have other code that assumes getMimeType() returns a string...
Consider
$mimetype = $file->getMimeType();
$mimetype = explode('/', $mimetype);
in
\Drupal\media\Plugin\media\Source\File::getThumbnail
This will also cause a deprection.
I think we should change \Drupal\file\Entity\File::getMimeType() to return the default value - or it could even do something like File::preCreate()... and guess the mimetype if we have a URI
/**
* {@inheritdoc}
*/
public function getMimeType() {
$mimetype = $this->get('filemime')->value;
if ($mimetype === NULL) {
$uri = $this->getFileUri();
$mimetype = $uri !== NULL ? \Drupal::service('file.mime_type.guesser')->guess($uri) : MimeTypeGuesser::DEFAULT_MIME_TYPE;
}
return $mimetype;
}
We should also update the interface to tell people not to return a NULL
I think this would be a good change because then all code can rely on getMimeType() returning something sensible rather than having to deal with the NULL situation (which must be very rare given the preCreate().
Only putting this in 11.x because while you could consider this a bug nothing is using this yet.
Committed df7ee1d and pushed to 11.x. Thanks!
Ticking issue credit boxes...
We need to add the new method to \Drupal\Core\Config\TypedConfigManagerInterface
I guess a year later I feel stronger that we should address this this way.
On the other hand this feels like the tail wagging the dog - we're making this change for testing purposes only - maybe there is a better way.
So looking at core and grepping for root.*>getPath\( reveals a few places the new method can be used...
So perhaps my instinct to add the method in 2024 was a good - let's do it. Especially since \Drupal\Core\Extension\Extension::__wakeup and \Drupal\Core\Extension\Extension::__sleep take care not to serialize the root.
I've reached out to @naderman and @Jordi Boggiano (maintainers of Composer) in Symfony slack to ask. No reply yet.
One thing I'm left pondering is whether this points to problematic design. Like instead of doing this should the stack work better with fibers and not allow the pollution - I'm not sure what that would look like but I'm guessing the problem is the static::$contextCollection.
I'm also wondering about other places in Drupal with similar logic that will need fixing for fibers - like the request stack.
@stephen-cox try adding container_rebuild_required: true to modules/localgov_media/localgov_media.info.yml - this should fix this.
This fixes a random error in a JS test in Thunder.