🇦🇺Australia @mstrelan

Annotated the MR to make it easy to understand why each of the param docs needed changing.

mstrelan → created an issue.

@quietone the sniff is already implemented in 📌 Require short ternary (Elvis operator) syntax Postponed

bbrala → credited mstrelan → .

You need to import the class.

mstrelan → created an issue.

If it helps for evaluation we already have a number of symfony polyfills and 10.3.x has symfony/polyfill-php83.

Is there a way to identify other places this could be used?

I didn't find an existing rector rule for this, the best I could come up with is so grep for foreach or for loops that have a return statement. If they are returning TRUE or FALSE then they might be candidates for array_all or array_any. If they return something else then possibly could be a candidate for array_find. I don't think it's worth trying to include too much in the initial scope.

#19 is spot on. Something Else (TM) is incrementing the assert count in functional tests, but I didn't dig enough to figure out what it was. Might be something in the mink setup or similar. We could probably replicate that behaviour by asserting something in the visit function, but that's a bit sneaky.

Thanks @catch. Re #16

Do we actually need different URLs for the visit calls at all, or could those just use '/' everywhere?

At least testAddressEquals needs the path set, the others could just use / or even NULL or '' probably.

Updating status on behalf of @quietone as per #13.

The upstream MR was merged yesterday so hopefully a new release shortly.

Added related issue ✨ Enforce return types in all new methods and functions Needs review for discussion.

Let's just review the idea and the changes to phpstan.neon.dist. If we're happy with that then we can regenerate the baseline. We can also review what's currently being added to the baseline, even if it's not mergeable, to verify that we're only identifier: missingType.return.

larowlan → credited mstrelan → .

Right, ideally this would be marked fixed and credit assigned to @fenstrat for the initial work 12 months before the D11 issue was created.

Looks like this might have been fixed in 📌 Automated Drupal 11 compatibility fixes for stable Fixed , can you please confirm?

It looks like the fix in 🐛 #states not working correctly when built from a logical combination of multliple fields Needs review was the exact same fix that had already been committed and reverted in this issue. So I'm wondering if we are still concerned about the issues in #34? I'm guessing not since @node_ was the one who committed it. Perhaps credit should be transferred from this issue as well. Setting to PMNMI to see if #34 still needs addressing.

Obviously it will need constant rebasing. Setting back to NR to get actual review.

Removed the bit about excluding tests. Tests call out to non-test code, and there can exist errors.

mstrelan → created an issue.

Why remove the return type instead of just making it nullable?

quietone → credited mstrelan → .

mstrelan → created an issue.

Opened ✨ Enforce return types in all new methods and functions Needs review

mstrelan → created an issue.

Re-ran the rector and decided to just git add core/*Test.php as that was easier than filtering out all the other paths, like traits, base classes, test modules, and random helpers in core/tests. That should address #6 and #7. We can discuss #8 in the parent.

Opened followup issue 📌 Refactor FormTestClickedButtonForm::buildForm Active

mstrelan → created an issue.

This is fixed by ✨ Conditionally disable access to update manager routes Needs work which will be released in 10.3.2.

Re: attributes solving this. This helps in that we that we can pass the class constant as the plugin id, but ideally we could drop the id property and just use getClass, or make them one and the same. See MR !8693 for a POC.

This is a duplicate of ✨ Support FQCN as plugin id Active .

Marking ✨ Use class names instead of plugin IDs Active as a dupe of this, although it has a POC with one plugin.

Any chance of a back port to 10.3 where the regression was introduced?

Forgot to push maybe?

Adding ✨ Use class names instead of plugin IDs Active as related, let's shoot for the moon.

mstrelan → created an issue.

Looks like it's failing.

Made the phpcs changes, let's see if it's green. IMHO if it's green then we've done everything in scope and any other refactoring can go to 📌 Fix usages of AssertContentTrait and stop recommending FormattableMarkup Active .

@smustgrave I thought @quietone was suggesting the threads in the MR should move to a follow up? But given the current state of strict types in tests I think we should add declare(strict_types=1) in this issue and remove <exclude-pattern>./tests/Drupal/KernelTests/AssertContentTrait.php</exclude-pattern> from phpcs.xml.dist. So leaving NW for that.

I considered changing the header, and it seems to work, but concerned it may lead to issues. We can also configure nginx to compress application/vnd.api+json, but ideally I'd like CloudFront to do it for us.

Happy for this to be closed, but leaving it open for now in case anyone has any novel ideas.

I don't see CR's for the latest 3 commits to phpcs.xml.dist, so maybe we're ok?

📌 Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard Needs work
📌 Fix 'Drupal.Commenting.TodoComment' coding standard Needs work
📌 [PHP 8.4] Fix implicitly nullable type declarations Active

Also in #3180696-89: Fix 'Drupal.Commenting.TodoComment' coding standard → @quietone mentioned CR is not needed.

Based on #17 I think this can be closed, given that Drupal 9 is no longer supported and no one has reported issues with Drupal 10.

I think I'd like to get ✨ Make $entityType optional: infer from class heirarchy Needs review in first. That refactors the hook implementation out of bca.module and in to its own class. It also has simple logic for setting the plugin ID to the class name, so perhaps we wouldn't need BcaDiscovery?

I think the priority stuff makes sense, I guess without BCA you'd have to put the logic in your own hook_entity_bundle_info_alter and probably also hook_module_implements_alter.

@abarrio thanks for working on this. The scope should be limited to tests only, so in paths like core/tests/* and core/*/*/tests. We also don't want to add union types in this issue, to keep the scope down, so should be limited to int returns only.

Kind of. There are some rector rules mentioned in the parent issue but they pick up more than just string returns so it's manually manipulated. I realise it probably hasn't found everything but I think it's reasonable to knock off this low hanging fruit.

Contrib doesn't use the phpcs file from core, but we could indeed add a CR regardless.

I guess that's better, but it would be good if you can see what $typed_config actually is where it's failing for you. It's not clear to me if contrib modules are doing something wrong, or if this post update hook is making incorrect assumptions about the config it's trying to update. If the latter, then we should try to fix it.

Changing title and component since this is not limited to Field UI. Confirmed it's the same behaviour on Claro, Stark and Olivero.

mstrelan → created an issue.

Slevomat coding standard was updated in #3262874: Update Coder to 8.3.15 → . Rebased and re-ran phpcbf. Back to Needs Review.

mstrelan → created an issue.

This will need a change record for the new dependency, but I'm not creating one right now until this is reviewed.

- I initially went for 'extending plugin attributes' but I think that's misleading because there is no class inheritance going on

Maybe not extending the plugin attribute, but you are extending the plugin. So maybe just rearranging the words to "plugin extension attributes" or something to that effect.

Needs work for the remaining functions in the Inspector class.

mstrelan → created an issue.

I think we should deprecate entityType in 1.2.x and remove in 2.0.x. Have added a separate MR for 1.2.x and updated the 2.0.x MR to remove the entityType param.

FWIW it seems CloudFlare supports the current mimetype https://developers.cloudflare.com/speed/optimization/content/brotli/cont...

mstrelan → created an issue.

IMHO we should add file_exists around every file that's loaded from the prepare script rather than having a special case for this one file.

@joachim that's exactly the method I'm talking about in #14. It should just fall back to the parent implementation.

Addressed feedback, please re-review. Considering changing the order of constructor params so bundle is first. That would give us the ability to annotate classes like this:

#[Bundle('article')]
final class Article extends Node {}

Deprecated the access check and added a CR. Not sure if versions are correct.

It seems a lot of effort has been put into breaking contrib compatibility here.

The CR that you've linked to here does not require you to use the attributes, you can use either attributes or annotations. This is in order to maintain contrib compatibility.

I tend to agree with TR on this issue. Perhaps it would be better to find a solution that deprecates rather than remove compatibility with popular contrib modules. If core is going to make breaking changes, why not do it in a major release?

Refer to #9. That said, I'm struggling to see why rules needs to override ::getDiscovery in the first place? The only difference I see is that it's not passing $this->additionalAnnotationNamespaces, so why not just make that property and empty array and let the parent class do the work?

Remove Doctrine from Drupal 11.

Doctrine annotations won't be removed in Drupal 11, that would be making the issue far worse.

Looks like it was 📌 Add a dictionary for Drupal-specific words RTBC

$ git log --oneline -S drupal-dictionary.txt
a10e0f8d1f Issue #3328741 by quietone, smustgrave, xjm, catch, alexpott, longwave: Add a dictionary for Drupal-specific words

Duplicate of 📌 Automated Drupal 11 compatibility fixes for bca Fixed

mstrelan → created an issue.

mstrelan → created an issue.

mstrelan → created an issue.

Closing this as no steps to reproduce have been provided after being asked in 2019, 2020 and 2023.

mstrelan → created an issue.

This is effectively the same as removing the setCacheMaxAge(0) call in UpdateManagerAccessCheck::access. Should we just remove that instead?

Good idea, but that causes issues with local tasks and local actions if the routes don't exist, and it seems like a lot of work to also conditionally create those. I've opened ✨ Conditionally disable access to update manager routes Needs work so we can discuss further. Postponing this issue on that one, there will probably be nothing to do here.

mstrelan → created an issue.

Opened ✨ Add a settings cache context Needs work which would also solve this, although it's entirely possible that is a bad idea.

mstrelan → created an issue.