Add symfony/polyfill-php84 and make use of new array functions

Needs work for the remaining functions in the Inspector class.

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

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

So not sure the process to evaluate this but looking at https://github.com/symfony/polyfill-php84

These are functions it claims to introduce

mb_ucfirst and mb_lcfirst
array_find, array_find_key, array_any and array_all
Deprecated

I don't see any open issues
Last release was June 19th, 2024 so still seems to be active

I personally don't see any downside,

Is there a way to identify other places this could be used? Looking at the file in the MR core/lib/Drupal/Component/Assertion/Inspector.php definitely seems cleaner

I'm a +1

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.

No sure who can make the call though.

Adding a backend dependency requires release and framework manager signoff.

Since this is a symphony package and we've used their polyfills before, it does not require a full dependency evaluation.

I'm personally in favor of the addition.

Yeah makes sense to me too - we can drop the polyfill once we require PHP 8.4, but we get the benefit of the new functions faster.

Added a change record, assuming it's too late for 11.0.

Think this can be RTBC? @mstrelan mind rebasing and I don't mind marking

Rebased / reran the composer commands

Thanks. Sorry if I’m jumping the gun but have only gotten +1s for this

I think @catch and I counts as RM signoff, so untagging.

+1 from me as FM to the concept

But needs work because the composer.json in lib/Drupal/Component/Assertion also needs to declare this dependency

Added the dependency as per #17, but unsure if I've done it correctly as the composer.json says it's partially generated automatically and I couldn't understand the link it points to → .

I guess we'd have to take care to add the same dependency to any other component that uses these functions.

Not 100% sure why if a core committer would know?

I broke this when I committed the pre-11.0.0 dep updates, so it will need to be re-generated.

Regarding the "why do we need to add the dependency" question: The IS of the issue it was for #3272110: Drupal 9 and 10's Drupal\Component composer.json files are totally out of date → explains the problem a bit better. Basically, before that issue, every component's composer.json was maintained by hand and so we got things like them being marked as version 8.8 when the core version was 9.4, declaring requirements of very old PHP versions that did not match core syntax anymore, etc. So the issue added ComponentGenerator, which does things like make the core versions match and reconcile the PHP requirement.

However, ComponentGenerator is not psychic; it can't tell that we're using PHP 8.4 syntax in this component now when core's own PHP requirement (and therefore all the components' PHP requirements) are 8.3. So to use the PHP 8.4 syntax, the component also needs to declare a direct dependency on the polyfill, in case it's installed by itself without core. Hope that helps. :)

I do agree that the CR could maybe use some work to be more specific about what ComponentGenerator does (and doesn't do). Also, it would actually be better for the generated message to link permanent documentation (rather than a change record). Let's file a followup to address that.

Looking at it, there's also a second problem that ComponentGenerator hardcodes PHP 7.3.0, rather than using Drupal's own minimum requirement. So that's a second followup issue needed. (It still would not address the issue of needing the dependency in the component, though.)

Opened follow ups 📌 Update package metadata readme to a docs page instead of a change record Active and 🐛 ComponentGenerator hardcodes PHP 7.3.0 Active

Rebased MR. Setting back to RTBC.

The validatable config job failed but that's because it's broken. Opened 🐛 Validatable config job is broken when MR introduces new dependencies Active .

I've just realised these array functions won't work on other traversables, only arrays, and that is a regression for the Inspector class.

In light of #24 I think we should revert the changes to the Inspector class and simply add the polyfill without any usage. Alternatively we could find somewhere else to use it.

Fixed CI as config validation job does not calling composer with new lock file after reverting commit

so now job has

Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 1 update, 0 removals
  - Downloading symfony/polyfill-php84 (v1.31.0)
  - Installing symfony/polyfill-php84 (v1.31.0): Extracting archive
  - Upgrading drupal/core (11.x-dev 71e78e0 => 11.x-dev 4a48295): Source already present

possibly, a rector rule could look for:
loops with a condition that returns the current element - array_find
loops with a condition that returns the current key - array_find_key
loops with a condition that returns FALSE - array_all
loops with a condition that returns TRUE - array_any

Changes still look good. Can the changes to the gitlab file though be reverted as seems out of scope for this issue.

Added polyfill without usage in 📌 Compatibility with Symfony 7.3 Active but not sure it will be accepted

btw there's 1.32 https://github.com/symfony/polyfill-php85

polyfill added via 📌 Compatibility with Symfony 7.3 Active

renamed and updated IS

Not sure if we need to do anything here or should just close it. The idea was to find a use case so we could add the dependency, but now it's been added anyway so no need to do this. Unless there is a rector for this maybe?

yep, rector rule + php85 sounds a good fit here

I had a look at the available rector rules for PHP 8.4 and none of them seem useful here.

• ExplicitNullableParamTypeRector - nothing to fix
• AddEscapeArgumentRector - nothing to fix
• RoundingModeEnumRector - enum not in the polyfill
• NewMethodCallWithoutParenthesesRector - can't use this in older versions of PHP

I don't think we need to refactor existing loops to use the new functions, I was only trying to do that to justify getting the polyfill in, but we have that now. I think we should just close this.

Agree that 📌 Compatibility with Symfony 7.3 Active got the polyfill in and feels like there's little point in searching for usages where we can use e.g. \array_find in old code.

Updating saved credits.

andypost → closed merge request !8655