Why is the typehint needed in core/modules/config/tests/config_test/src/Entity/ConfigTest.php ?
This was done at the suggestion of @larowlan in comment #6. And it's at the core of what this issue was originally created about. From the issue summary:
[The test] attempts to blindly set all required properties to NULL and assert a validation error. But if the property being set is typed (i.e enforced at a language level) then setting it to NULL makes the test fail with a TypeError...
In order to test this problem we need to have a typed property.
Currently, if the $entity is NULL then editTitle() returns NULL and deleteTitle() returns an empty string. This is separate from what happens if label() returns a value or NULL. If we want to maintain the current behavior of the functions, then we have to handle the case of a missing entity.
Sure thing. I didn't think about it the other day. I was ready to move on to something else after hours of frustrating debugging.
I took a quick look at the MR and noticed that the new update path test doesn't have the newest attributes. I suggest rebasing it and updating the test.
Yeah. Running the RDF test with these changes triggered the deprecation warning as it should. So I'll set this to RTBC.
The rebase was done. Tests are passing. I'm restoring the RTBC status per #11.
My mistake.
I searched contrib for uses of both functions and turned up one result. RDF extends CommentTestBase and uses setCommentAnonymous(), which makes sense because it was in Core. See https://git.drupalcode.org/project/rdf/-/blob/2.x/tests/src/Functional/C....
Following on from the review in #14, I'm guessing that the question at hand is whether it's OK to type hint parameters or return values with an enum, but if I'm mistaken then please let me know. I checked the @param and @return data types and researched the answers.
The enum documentation page shows an example of a function parameter typed hinted with an enum.
The ::tryFrom() docs themselves show a return type of static|null, so it's OK to type hint a return value with an enum.
They look good to me. I'm going to RTBC this.
I did manual checks on all the changes. Overall they look OK to me. I left comments for the couple of things that I noticed.
Updates to type hints in documentation are typically classified as tasks, not bug reports. This doesn't need tests.
As suggested, I'm closing this issue. There is an
existing issue
📌
Replace non-test usages of \Drupal::request() with IoC injection
Needs work
for converting calls to \Drupal::request() to container injection. So I'm considering this to be a duplicate.
core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php also has a call to \Drupal::request() as noted in
🐛
Call to a member function has() on null in Drupal\Core\Mail\Plugin\Mail\PhpMail->mail()
Active
.
I made the suggested changes, but had to open a new branch and MR because I messed up the rebase.
dcam → changed the visibility of the branch 3446368-symfonymailer-multiple-recipients to hidden.
But I'm really not keen on the mixing of if/else and ternary that there is now.
Yeah, it was kind of ugly. Unfortunately, the nullsafe operator doesn't help here because the functions don't return values based only on the label. They also return something in the case of NULL entities. Still, I didn't try to clean up the callbacks before, so I did it now. I'm not sure that I like this much either, but at least it should be easy to understand.
Writing the test was WAY more of a pain than it should have been.
First off, I uncovered two additional bugs in this test! The class contains two mentions of the required_label data type, including where it says:
// If a config entity type is not fully validatable, all properties are
// optional, with the exception of `type: langcode` and
// `type: required_label`.
Except it doesn't actually check for required_label data. In both places it just assumes that all labels are required. So if you're validating a config entity that isn't required, like config_test, then the test fails. I couldn't simply change the config_test label's data type to required_label either. When I did a bunch of tests started failing.
To solve this problem I updated ::testRequiredPropertyValuesMissing() and ::getPropertiesWithOptionalValues() to verify that a label is required before treating it that way.
Secondly, the weight couldn't simply be given an int type (see #6). Tests started failing when I did that. I tried other solutions like assigning types to other untyped properties and adding another property. You can see the commit log above. Everything I tried broke tests. It turned out that adding the type to weight broke the least amount of tests, just one. It was the ConfigEntityAdapterTest which tried to assign a string value to the weight to trigger validation errors. Of course, with the new type this can't be done at the PHP level. I changed the adapter test trigger a violation on the untyped style property instead. I don't think this is important to the test, but please double-check.
@smustgrave I don't think so. The constraint validator seems incompatible with form validation as far as I can tell. Did you have any thoughts about how?
I updated the change record with information about the automatically-assigned categories.
I haven't invested time in this one because I'm pretty sure there's another issue about orphaned entity references somewhere else. I've been trying to find it. Also, I do not like the fix. Replacing the reference with <none> means that if the entity is saved without someone noticing that the reference changed, then you have data loss. I'm pretty sure the other issue I mentioned was hung up on that point too.
The only reason I updated this issue last week was because it was assigned to the wrong component, but I really didn't intend for anyone to work on it. I've looked for the other issue since then, but haven't been able to find it. Assuming that I didn't imagine it, then we need to find it, evaluate any existing duplication between the two issues, and then figure out if the efforts should be combined.
I added a change record at https://www.drupal.org/node/3557004 → .
@r_mahi please read https://www.drupal.org/community/contributor-guide/contribution-areas/core → . In particular there is a link to the Drupal #contribute Slack channel. I encourage you to seek assistance there for getting started. Right now Drupal is in the middle of transitioning from its custom issue queue to GitLab. This comes with particular challenges when learning how and where to do things. Putting that information into an issue comment isn't appropriate. It would be best if you could use Slack to communicate directly with other contributors for assistance.
I added a unit test. I'm not necessarily happy with it, but this was the best idea that I had.
I converted the patch from #13 to an MR and added a Unit test. Then I realized that EntityController::deleteTitle() has the exact same problem. So I expanded the scope to include it.
It's a bit odd. ::deleteTitle() returns an empty string if the entity can't be loaded. Whereas ::editTitle() returns nothing (a NULL value). My new test does not cover these cases, partly because it would require different mocking and partly because I feel like making the two callbacks behave the same maybe ought to be handled in a follow-up issue. In which case the NULL entity test cases can be written at that time. Not my call though and that's WAY out of scope for this bug fix.
This looks like a duplicate of
🐛
Entities without canonical link template throw UndefinedLinkTemplateException if comments are attached and rendered
Needs work
. They're modifying the same place in CommentForm in similar ways, but the other issue's MR does much more. Since that's the case and the other issue is also older, I'm going to close this one. Credit has been granted for writing the patch.
This is a feature request masquerading as a bug report. A problem caused by custom code trying to implement something that isn't supported by Drupal Core does not mean that there is a bug in Core. Thus, I am changing the issue category and title.
I'm tagging the issue as needing subsystem maintainer review to get buy-in from them (or not) for this new feature.
Per #8 I've assigned credit and am closing as a duplicate.
I couldn't get it to fail locally with PHP 8.5 either. But afterward I realized that we have another potential conflict with the upcoming Drupal.Array.Array.LongLineDeclaration sniff.
I just noticed 📌 Move global comment permissions to comment-type level Needs work which if implemented would cause this test to need to be updated again. I don't think a 12-year-old issue that isn't approaching a resolution should hold up this task that's necessary for getting Contact out of Core. But it does need to be mentioned.
Before the fix, executing the test resulted in four deprecations:
laptop:~/drupal_core$ phpunit core/modules/media/tests/src/Kernel/MediaSourceTest.php
PHPUnit 11.5.42 by Sebastian Bergmann and contributors.
Runtime: PHP 8.5.0RC3
Configuration: /var/www/html/phpunit.xml
........DD 10 / 10 (100%)
Time: 00:11.784, Memory: 6.00 MB
2 tests triggered 4 PHP deprecations:
1) /var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php:356
Using null as an array offset is deprecated, use an empty string instead
Triggered by:
* Drupal\Tests\media\Kernel\MediaSourceTest::testDifferentSourceFieldDisplays
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:616
* Drupal\Tests\media\Kernel\MediaSourceTest::testHiddenSourceField
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:637
2) /var/www/html/core/modules/field/src/Hook/FieldHooks.php:229
Using null as an array offset is deprecated, use an empty string instead
Triggered by:
* Drupal\Tests\media\Kernel\MediaSourceTest::testDifferentSourceFieldDisplays
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:616
* Drupal\Tests\media\Kernel\MediaSourceTest::testHiddenSourceField
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:637
3) /var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php:372
Using null as an array offset is deprecated, use an empty string instead
Triggered by:
* Drupal\Tests\media\Kernel\MediaSourceTest::testDifferentSourceFieldDisplays
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:616
* Drupal\Tests\media\Kernel\MediaSourceTest::testHiddenSourceField
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:637
4) /var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php:374
Using null as an array offset is deprecated, use an empty string instead
Triggered by:
* Drupal\Tests\media\Kernel\MediaSourceTest::testDifferentSourceFieldDisplays
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:616
* Drupal\Tests\media\Kernel\MediaSourceTest::testHiddenSourceField
/var/www/html/core/modules/media/tests/src/Kernel/MediaSourceTest.php:637
Afterward there were no deprecations. I double-checked the deprecation backtraces and verified that they're all due to missing bundle IDs. I really wish we could add parameter type hints to EntityFieldManager::getFieldDefinitions(). /sigh
But this needs a code style change to avoid conflicting with Drupal.Array.Array.LongLineDeclaration which is in an RTBC MR.
I'm actually not that familiar with the Comment module, so I had to double-check that there are no bundle-related permissions. There aren't. Using Comment here seems like a good, straightforward replacement. And the test is still passing. This looks good to me.
I took a shot at adding a CR. Please review https://www.drupal.org/node/3556699 → .
The changes look good. I don't have any feedback. grep -r "@covers" ./core only returned instances in ./core/tests/PHPStan files.
Some research probably needs to be done why it’s an array here.
Commerce does it a lot, apparently. I searched Commerce core for the setting name and it turned up a number of results, see https://git.drupalcode.org/search?search=allowed_values_function&nav_sou.... That fact makes you wonder: if they're doing this so much then why aren't we getting more bug reports about it? I think it's because functionally there's really no problem with it. The way that options_allowed_values() is written people should be able to use the array syntax for first-class callables. That's despite the fact that the module doesn't explicitly support it: the array syntax shouldn't be able to be stored in field configuration which only accepts a string.
So, what's going on here? The error message in the issue's title and summary gives a clue. It says that the error occurs when function_exists() is called on line 90 of options.module. Except function_exists() is never called in options.module. I checked back to version 9.0.x. It's never been in there. We need to know: was this file modified through other means? Based on the available information it sounds like that's the case.
Did anyone search for other instances?
Yeah, as part of #5.
MR 10503 was opened against the incorrect branch. It had to be rebased manually. So I decided to open a new branch instead and manually copy the changes. I also added a test.
dcam → changed the visibility of the branch 3492882-fix-schema-contenttranslation to hidden.
I checked the diffs of all of the changes. There wasn't anything missing or weird. It looks good to me.
I can't reproduce this issue. On a Drupal site with the latest code I was able to successfully translate the "item(s) selected" text in the media library widget. I tried to track down which commit was responsible for fixing the problem, but was unsuccessful. If you believe that this issue was closed in error, then feel free to reopen it, but you'll need to provide more thorough steps to reproduce the problem from a fresh installation of the Standard profile.
A nearly identical change was introduced by 📌 Move Olivero's messages.css to its messages library Active and committed earlier this year. This issue is outdated.
I'm closing this issue as being unable to reproduced. @rastepanyan, please be aware that steps to reproduce an issue should be given starting with a fresh installation of Drupal's Standard profile. The brief text you gave seems to be based on a customized site and probably involves contributed modules being installed. It is insufficient for us to reproduce or even understand the issue. Frankly, the information given doesn't even sound like it has anything to do with the subsystem being patch, which is the Link module.
Furthermore, the LinkWidget class does not call ::isTranslatable() on node objects and never did at the time you posted this issue, but I notice that your patch does. That makes me think that you modified the LinkWidget class for some reason relating to translations, then posted this issue about a problem those modifications caused. That's why I'm closing this issue.
If you believe that this issue is being closed in error, then feel free to reopen it, but not without accurate, itemized steps to reproduce the issue beginning with a fresh install of Drupal Core.
This is a duplicate of ✨ Allow to configure translation per property on link fields (for translating only the title, not the url itself) Postponed: needs info , which already has work done in an MR.
This code got moved around, but ultimately 🐛 Module constraint checks fail incorrectly due to str_replace Fixed applied the exact same proposed fix to it. This issue is outdated.
This is failing for the exact same reason as 📌 Fix account mock in \Drupal\Tests\menu_link_content\Unit\MenuLinkContentEntityAccessTest::testUnrecognizedOperation for php 8.5 Needs review , which got fixed and committed today. I reviewed that issue too. The fix for this issue is the same as the one that went in the other. It still looks good to me.
This was a duplicate of 📌 Cleanup get_value_options by returning the value options Fixed which was committed, fixing the issue. This is outdated.
I'm happy to add screenshots so this issue is clearer. It's done.
Based on the comment in the test I'd expect these to fail but they pass...
No, they're passing appropriately. There's no issue with those elements. In other words, they're being hidden appropriately. But there's no test coverage for it. I could have left those assertions out because they have nothing to do with this issue. But they were easy to add along with the assertion for this issue. Just consider it to be bonus coverage. See the before image I've added. There is no "New set" radio button or "Title" text field.
But comments should also be updated.
I assume you're talking about this comment:
// Verify that users without the "administer shortcuts" permission have
// access to the set selection radios, but not the new shortcut form
// elements.
Users without that permission shouldn't be able to see the "New set" radio, "Title" text field, or "Machine-readable Name" field. These are the elements related to adding a new shortcut set. Would you prefer alternative text that explains it in a different way?
There isn't much to say about this. I verified that the test was failing due to deprecations before now. But the test passes in this pipeline. I verified that the fix is appropriate. The point where the class throws the deprecation error is checking the page.front config key. So if it isn't set properly, then it would return NULL like the failures say. Looks good to me.
Think if someone incorrectly passes a string we should maybe log or throw an error so people know they need to fix their code.
I disagree. This is a validation function. It seems inappropriate for validation to throw an error if something doesn't validate. Logging, maybe, but I think even that's a stretch. It should be up to calling code to decide what to do if validation doesn't pass.
The only place in Core where this is called is in DateTimePlus::createFromArray() in the same class. ::createFromArray() throws an exception if validation failed. So it's doing the job.
The patch from #2 was converted to an MR. Later patches included unrelated code style changes. I also added new cases to the existing test data provider.
I am not able to reproduce this issue using the steps given in #11. The other related issues have also been closed as being outdated because the issue is considered to be fixed. So I'm doing the same here.
I disagree with the approach taken in #2. That would only hide the element visually. What we really need to do is set the #access on the element so it's removed from the form entirely for those who shouldn't have permission to see it. In fact, this is sort of a security gray-area. Users have the ability to see something that they shouldn't. Fortunately, they can't do anything with it. Entering text into the element does nothing. Otherwise I'd be contacting the security team. I considered adding the Security Improvements tag, but decided not to.
I added test assertions in addition to changing the proposed fix. I also updated the issue summary.
This looks good to me. I verified that MenuLinkContentEntityAccess is currently failing in the PHP 8.5 job, see a test run from another issue I worked on earlier today. The failing test shows that the problem occurs on line 221 of core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php. This is where id() is called. So mocking id() is an appropriate fix. And the fixed test passes in the pipeline here.
I converted the patch from #7 to an MR with some modifications.
This feels like a case for an \UnexpectedValueException to me instead of a \LogicException.
I added a condition at the top of the function for returning a zero value in the case of an empty string. We specifically test for this case (see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/...) and it has to be preserved. I don't like it, but there's no telling what core, contrib, or custom code may be relying on this special case. So I figure it's best to not mess with it. The new exception stopped this case from working, so it has to be handled before the input parsing starts.
I tweaked the exception text a little.
I removed the changes to the else {...} condition at the end of toNumber(). I agree that it's pointless, but it's out-of-scope.
I also added a unit test.
I cancelled that most recent pipeline because there was no need to waste electricity for testing a typo fix. See https://git.drupalcode.org/issue/drupal-3308418/-/pipelines/646508 for the passing results from the previous change which added tests.
I'm recategorizing this as a task. #3013802: Improve error message on unrouted URLs → on which this was based was also a task.
I'm also expanding the scope of the issue to include Url::setRouteParameters() and Url::setRouteParameter(). These functions have the exact same error message about "external" URLs.
I did add more test coverage for the case of unrouted Url objects executing ::setRouteParameters() and ::setRouteParameter(), but beyond that we don't need more coverage. url::getRouteParameters() already has coverage for the unrouted case in UrlTest::testGetRouteParametersWithExternalUrl(). And we generally don't test for specific exception strings, only the exception classes.
dcam → changed the visibility of the branch 3308418-improve-error-message to hidden.
This is used in seven in contrib. Know that's not core but a lot of sites still use that theme. So what's the concern there if we just remove vs deprecate?
@smustgrave I took a peek at this issue and wondered wondered how Seven was using the library. But I can't find a reference to either removed library in there. I see Seven's own node library. I've triple-checked, but I admit that maybe I'm just clueless. Still, I wanted to mention it in case you were mistaken. If I've just overlooked it, then please feel free to let me know.
These changes need to be rebased for Drupal 11. The Seven theme is no longer even in Drupal Core. Usually the easiest way to do this is to create a new branch and MR based off of 11.x instead of changing the current MR's target version. Then hide the old branch.
Using the command git diff bf76a69d I verified that the code is the same as that which was RTBCed back in March 2025 in comment #128. Subsequent changes to adjust the ScriptBytes in performance tests have been reverted. I'm restoring the RTBC status.
Not sure what that was about, but I'm restoring the RTBC status.
I know you said to not put much time into it, but I did anyway.
I checked the diff of the two files against each other to make sure changes were copied correctly. And they look OK to me with the exception of the one thing I made a comment on.
Following that, I went through the original version of SystemRequirements and hunted for all the phase conditions. I verified that all the runtime and update phase conditions went into the new file and only the install phase conditions stayed aside from the things that belonged to both.
Aside from the comments on the MR this is looking good to me. I think we're OK to proceed with this as the way forward instead of option 2.
Thank you for your suggestion to improve Drupal! I've reviewed the change, but I have a concern about it. The new URL's destination is not equivalent to that of the old URL. The old URL specifically went to the documentation sub-page about templates. Whereas the new URL goes to the top-level documentation page.
Also, when I grepped core for "twig.symfony.com" I found a bunch of other Twig 3.x documentation links. A few years ago most other links were updated to their 3.x equivalents. So there's a precedent to consider. My suggestion is to do the same here.
I manually verified all recent changes since my last review, comparing provider return values to the new test definitions. They all look good to me.
I've been following this issue for years longer than I've been the Link module maintainer. I've also wanted to set a default text value for links while letting the URLs be customized per node. Now that I think about it, I still manage that application where I wanted that ability.
That said, I understand the potential usability issues at play here. I started thinking about other possible solutions and came to a realization that ironically was suggested back in 2016 in comment #43 by @Ilesyt:
I would like an option where could I input a link text in the 'Manage display' when editing the field display.
The need that this issue is looking to solve would be really easy implement with a field formatter that has configurable default text. And wouldn't you know it, there's a module that does it: Fixed text link formatter → . So if you need default link text functionality, then I advise looking into it.
I see from the comments that naming the property full_url wasn't necessarily a popular choice. I'm not fond of it either. It isn't descriptive or meaningful.
A resolvable, or dereferenceable, URI is a URI that actually points to a functioning website.
I renamed the property from full_url to resolvable_uri. I wouldn't mind resolvable_url, but I decided to stay with "uri" to keep the association with the original uri property. Feel free to provide feedback or other suggestions.
I recreated the MR for Drupal 11. The original MR was hidden and a new one was created because usually changing the MR version breaks everything. Recreation is often the easiest way forward. I had to fix an issue with the JSON:API ShortcutTest. There's a comment on the MR about it.
The issue summary and change record have been updated.
I don't think there would be BC issues since this is autowired and probably no one is extending this, but switched it over anyway.
Me either. But who wants to deal with that one site that has it overridden and shows up to find out who broke their stuff?
Feedback has been addressed. Happy to RTBC this now.