🇺🇸United States @dcam

This is close to being ready. I don't have any additional feedback beyond the two existing MR comments. Manual tests worked without a problem. Files in the cache directories older than 6 months were removed on a cache clear. Newer files were retained. I read through the CR and it looks good.

The Problem/Motivation section of the issue summary says this:

Given all the above, it think we should consider re-introducing the stale file threshold concept, and only delete asset aggregates that are over 30 days old by default.

I'm guessing the "30 day" threshold is out of date since everything else says 180 now. If it's wrong, then it should be updated.

Settings to Needs Work for the MR comments and to have the IS checked.

There are a couple more changes needed.

I checked the regex in the edited ignore statements based on the feedback from #34. The statements now target specific namespaces and classes in Drupal Core. So the feedback was addressed.

The latest commit looks good. It's a straightforward change. And tests are passing, so the linting agrees. Back to RTBC.

Shall I change both CRs adding the constructor examples?

No, if someone wants it to be added to the old CR, then they can handle it themselves. It's out-of-scope for this issue.

For what it's worth, I've been instructed to add examples to all my CRs lately, even when I didn't think it was really that necessary. And a search for "new parameter" in the CR list → shows the majority of the recent ones have examples.

I think the code changes are OK, although it's probably too late to get this into 11.3. But the change record needs before and after code examples of calling the constructors.

This behavior is controlled by the field system and is not specific to the Link module. I believe this issue belongs in the field_ui.module component.

Also, issues must be fixed in the current development branch which is 11.x.

I ran PHPStan before and after the changes. I verified that the changes reduce the violation count by 90. My recent feedback was addressed. Tests are passing. This looks good to me.

The new change makes sense - that's a constant after all. After downloading the latest changes PHPStan reported no errors.

These changes are looking good. I read through them all and verified all the file, route, and library moves. Then I double-checked them in the browser. All the routes and libraries loaded properly.

This needs a manual rebase due to changes in the PHPStan baseline.

I ran PHPStan with the rule turned on. I only got 16 errors instead of the 19 mentioned in the IS. I don't know if other recent updates fixed a few of them. The MR corrected all of the issues PHPStan found.

Afterward I read through all of the changes. They all seem reasonable. I would leave the int casts in this MR rather than splitting them off, especially the ones from Workspaces. I left a couple of replies to the comments, agreeing with the assessments.

This one's looking good to me. I'll RTBC it.

Personally, I don't like the possibility of having a single radio element as in the case of there not being an edit-form link template. I don't know if that was considered when the usability review was done or if it matters.

Was it a conscious decision to continue hiding the link form elements if there is no canonical link template? There's no discussion about it. This feels like a another issue waiting to happen when someone eventually wants to link to an edit-form route for an entity type that has no canonical route.

...yes, a module could extend this by implementing hook_config_schema_alter() to change the options available to the validation constraint, then probably a targeted form alter to put additional options into the settings form.

This was the response to the question of whether other link templates could be added as options without having to override the class. If I'm not mistaken, then this isn't true anymore. It's caused by the match() statement in settingsSummary(). If someone altered the form to add another template, then the summary would throw an UnhandledMatchError on the display form. Is there a way around it?

The "Update Fork" button worked.

My feedback was addressed. It looks good to me.

Thank you for instructing me on that. I'm going to go back and look for it when I'm home later.

Ugh, thank you. I could not figure out where that was coming from. I checked the output multiple times, but never saw any indication. IDK if I just missed it or didn't understand what it was saying.

I was shocked when I saw how many queries this saves. Well done finding it.

...we also have to translate English when locale's 'translate_english' setting is on, even when it's the default language - at least as far as two tests are concerned.

Thank you for documenting this in a comment because I didn't understand that condition until I read #12.

My main concern is the code duplication from the new conditions in LanguageConfigFactoryOverride, especially given that the translate English condition is a bit unintuitive. Might it be a good idea to move these to a new function on the class that will return a boolean?

Do the changes to LanguageConfigFactoryOverride::loadOverrides() and LanguageConfigFactoryOverride::getOverride() need unit tests?

I had to do a major reconfiguration of the tests. The test plugins were originally put into a module that had a dependency on entity_test. It seems like entity_test has been updated recently, which caused the fixture to be out of date. That would be a perpetual problem. We can't have that. So I moved the plugins to an isolated test module without a dependency.

I reverted the changes to core/modules/filter/src/Plugin/migrate/process/FilterID.php which was recently deprecated. That deprecation causes PHPStan issues in core/modules/filter/src/Plugin/migrate/process/FilterID.php when FilterID::create() is removed. I couldn't stop the errors. If someone has an idea, then I'll try to fix it. But for now I decided to revert the changes since the class is deprecated anyway. So I'm not sure it matters.

I sat down to write the change record and then realized that node.module.css isn't removed from the Node module directory. Is this correct or should it be deleted too?

Setting to Needs Work for the change record.

Rebased. The conflict was in stable9.info.yml due to the addition of Navigation libraries.

I can't wait to remove the workaround from my theme!

This looks great. The issue summary contained all the information I needed to understand the issue. The edited docblock contains correct information now. The change record URLs point to the correct place. I edited the CR a little to fix a typo and add before/after examples.

We often use @trigger_error(__METHOD__ . "() is deprecated..."); when deprecating a class method so its FQCN and name are dynamically included. But I don't consider this to be a reason to set the issue back to Needs Work status. I'm only mentioning it so people can learn.

The contextual module toolbar button was omitted from Navigation without replacement. See 📌 Integrate Navigation with Contextual editing Active . So that help text was removed entirely.

dcam → made their first commit to this issue’s fork.

I verified that all #[Autowire()] attributes have the service named parameter. Feedback has been addressed. I'm moving this back to RTBC.

Feedback was addressed. This looks good to me.

Feels weird to be thanked for something that seemed kind of nit-picky. But anyway, the latest changes look OK to me now. Back to RTBC.

Setting to Needs Work due to a small consistency issue that I noticed in the latest commit. Feedback is in the MR comments.

I left some feedback and a change suggestion.

The feedback was addressed. I don't have anything to add. So I'm going to move this back to RTBC.

I ran PHPStan as shown in the issue summary on my local and got the same 17 errors. After applying the MR all 17 errors were gone. I read over all the changes and didn't see anything weird. For the record, core/modules/image/tests/src/Unit/ImageStyleTest.php, which is at the top of the MR changes list, is the file where we're fixing the MockBuilder mentioned in the IS.

It looks like we're also standardizing the docblocks and type hints of all properties in the classes being edited. I think that's a good thing to help prevent something like this from happening again.

With all that said, this looks good to me.

ErrorTestController was originally included, and it caused test fails, so I reverted it.

Obviously I didn't look at the commit history. Sorry about that. In that case, let's roll with it. The new changes look good to me. My feedback was addressed.

Rebased. Restoring the RTBC status.

I created the change record at https://www.drupal.org/node/3559908 → . In this case, there isn't anything that implementing code needs to change, so I didn't include before and after examples.

PHP 8.5 introduces a new always-available Uri extension for parsing and manipulating URIs. Ref:

• 8.5 New Features (PHP Manual)
• Announcement on The PHP Foundation blog
• RFC

It's possible that we may be able to replace some of our custom URL parsing and validation with the new extension. This could start during the Drupal 12 lifecycle if we maintain our policy of requiring the most recently released version of PHP → . My hope is that it would allow us to close many of these issues that remain open.

As the Link module maintainer this effort would impact the work that I'm doing. So I'm willing to take the lead on the project.

Closing since there was no response.

I looked over the MR. I agree with the other comments that were made and left suggestions, so I'm setting to NW. Mainly, that assertTrue() is dead code.

Feedback was addressed.

Fair enough. I thought they might have been in-scope.

Sure if you want to spin up the separate issue I can revert here tomorrow

I created the issue 📌 options_config_install_test has unnecessary Body field Active .

dcam → created an issue.

The rest of the changes look good to me, but when I ran a check for any missed text-with-summary-related stuff in core/profiles and core/recipes I found some mentions in demo_umami template docblocks. The files are:

• core/profiles/demo_umami/themes/umami/templates/classy/field/field.html.twig
• core/profiles/demo_umami/themes/umami/templates/components/field/field--node--field-cooking-time--recipe--full.html.twig
• core/profiles/demo_umami/themes/umami/templates/components/field/field--node--field-difficulty--recipe--full.html.twig
• core/profiles/demo_umami/themes/umami/templates/components/field/field--node--field-number-of-servings--recipe--full.html.twig
• core/profiles/demo_umami/themes/umami/templates/components/field/field--node--field-preparation-time--recipe--full.html.twig

You can also find them with a regex search for "text[-_]with[-_]summary".

I found it unusual that an Options module test fixture has Body field configuration. So I stripped the body field out of the fixture module and ran the Options tests. They all passed. I'd say we should get rid of the field entirely. My suggestion is that you revert the changes to the Options test config here and we open a separate issue for removing the field. Since I already have the changes pending here on my local environment I'd be happy to create the MR.

I have no idea what was going on with it. The "Update fork" button worked on the branch page. It's reporting as being mergeable now.

I left some feedback. It looks close to being ready.

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.

dcam → made their first commit to this issue’s fork.

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 11.x to hidden.

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.

dcam → made their first commit to this issue’s fork.

I added a change record at https://www.drupal.org/node/3557004 → .

dcam → made their first commit to this issue’s fork.

@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.

dcam → made their first commit to this issue’s fork.

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.

dcam → made their first commit to this issue’s fork.

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.

I added a unit test.

dcam → made their first commit to this issue’s fork.

Per #8 I've assigned credit and am closing as a duplicate.

I added tests. I think this one is ready for review.

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.

I converted #19 to an MR and added a Kernel test.

dcam → made their first commit to this issue’s fork.