This is to see part of the impact ofphp/php-src#18879, and it seems to be finding some bugs in tests or SF itself, but I would need confirmation from other people.

This is very much W.I.P. and I might use CI to check that my changes are correct as I have some local test failures even just running with 8.4.8

I am well aware the commit messages are kinda useless at the moment, but they are somewhat self-contained, so they can be looked at individually.

Copy link

carsonbot commentedJun 24, 2025

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

Girgias added6 commits

June 24, 2025 13:39

Pull fixes from Nicolas

1ac3888

Pull ignores/suppressed tests from Nicolas

2db7dac

Use false instead of 0

9e73d6a

IntlTestHelper::requireFullIntl() use null as second arg instead of f…

d388344

…alse

Check return value of getenv()

4923fa4

Symfony\Component\Validator\Constraints\Isbn does not take a bool as …

1cefb6b

…first arg

stof reviewed

Jun 24, 2025

View reviewed changes

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php Outdated

		@@ -568,7 +568,7 @@ private function parseDefinition(string $id, array\|string\|null $service, string
		if (isset($call['method']) && \is_string($call['method'])) {
		$method = $call['method'];
		$args = $call['arguments'] ?? [];
		$returnsClone = $call['returns_clone'] ?? false;
		$returnsClone = $call['returns_clone'] ? true : false;

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this will break ifreturns_clone is not defined in the array.

and that's a case where we expect a boolean if the key is defined, or an undefined key (which should then default tofalse)

src/Symfony/Component/DependencyInjection/Definition.php Outdated

		@@ -307,7 +307,7 @@ public function setMethodCalls(array $calls = []): static
		{
		$this->calls = [];
		foreach ($calls as $call) {
		$this->addMethodCall($call[0], $call[1], $call[2] ?? false);
		$this->addMethodCall($call[0], $call[1], $call[2] ? true : false);

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this is a case where the third value in the array is expected to be a boolean or absent, which is broken in this change.

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

		$definition->addMethodCall(
		$call->getAttribute('method'),
		$this->getArgumentsAsPhp($call, 'argument', $file),
		(bool) XmlUtils::phpize($call->getAttribute('returns-clone'))

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I suggest doing a method handling of the case of the missing attribute (instead of handling it as$call->getAttribute('returns-clone') returning an empty string which then casts tofalse)

Copy link

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you want me to basically just do:

Suggested change

	(bool)XmlUtils::phpize($call->getAttribute('returns-clone'))
	XmlUtils::phpize($call->getAttribute('returns-clone')) !==''

or something else? Because I don't really understand your suggestion.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Doing something like$call->getAttribute('returns-clone') !== '' ? XmlUtils::phpize($call->getAttribute('returns-clone')) : false, as we need to detect the case of the omitted attribute (to default tofalse) while still using the proper output ofphpize for cases using the attribute.

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php Outdated

		@@ -589,7 +589,7 @@ private function parseDefinition(string $id, array\|string\|null $service, string
		} else {
		$method = $call[0];
		$args = $call[1] ?? [];
		$returnsClone = $call[2] ?? false;
		$returnsClone = $call[2] ? true : false;

Copy link

Member

stofJun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the existing code should probably be kept here as well.

stof reviewed

Jun 24, 2025

View reviewed changes

src/Symfony/Component/ErrorHandler/ErrorHandler.phpShow resolvedHide resolved

Copy link

Member

stof commentedJun 24, 2025

Some of those changes are definitely legitimate as they are clearly mistakes in the existing code that went unnoticed (static analysis could catch them, but our CI only reportsnew violations in each PR and we never worked on fixing issues reported in existing code):

the wrong parameter passed in the instantiation ofIsbn
the wrong type passed toIntlTestHelper
the wrong type passed tostream_set_blocking
the wrong parameter type in the internal method of GraphvizDumper

Those could definitely be submitted directly (against the 6.4 branch).

We have other cases (when using preg_match or bitwise comparisons) where we indeed rely on the implicit type juggling of the return type, which are legitimate changes once PHP deprecates those type juggling rules (static analysis probably also prevents them for new code).
As we will want to support PHP 8.5 in the 6.4 branch, fixes for them should also be submitted against 6.4 (I would suggest separating them from the ones fixing clear mistakes).

As reported in my review comments, the changes done in the DI component look wrong to me (and are the cause of failures in the CI as well)

Copy link

Author

Girgias commentedJun 24, 2025

Some of those changes are definitely legitimate as they are clearly mistakes in the existing code that went unnoticed (static analysis could catch them, but our CI only reportsnew violations in each PR and we never worked on fixing issues reported in existing code):
* the wrong parameter passed in the instantiation of `Isbn`* the wrong type passed to `IntlTestHelper`* the wrong type passed to `stream_set_blocking`* the wrong parameter type in the internal method of GraphvizDumper
Those could definitely be submitted directly (against the 6.4 branch).
We have other cases (when using preg_match or bitwise comparisons) where we indeed rely on the implicit type juggling of the return type, which are legitimate changes once PHP deprecates those type juggling rules (static analysis probably also prevents them for new code). As we will want to support PHP 8.5 in the 6.4 branch, fixes for them should also be submitted against 6.4 (I would suggest separating them from the ones fixing clear mistakes).
As reported in my review comments, the changes done in the DI component look wrong to me (and are the cause of failures in the CI as well)

Yes I had figured out the cause, but I was having some other local failures before that made it difficult to waddle through stuff. There is also a bug in master on php-src which is not helping me to get a good overview of the situation.

Question aboutIsbn should I drop thenull argument as it's the default or not?

carsonbot closed this

Jun 24, 2025

nicolas-grekas reopened this

Jun 24, 2025

Girgias force-pushed thebool-type-juggling branch fromd89795e tob812e26Compare

June 24, 2025 13:33

Girgias marked this pull request as ready for review

June 24, 2025 13:35

Girgias requested review fromlyrixx,yceruto,kbond,dunglas andxabbuh ascode owners

June 24, 2025 13:35

carsonbot added the Status: Needs Review label

Jun 24, 2025

carsonbot added this to the7.4 milestone

Jun 24, 2025

symfony deleted a comment fromcarsonbot

Jun 24, 2025

symfony deleted a comment fromcarsonbot

Jun 24, 2025

stof reviewed

Jun 24, 2025

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckTypeDeclarationsPassTest.php

		@@ -336,7 +336,7 @@ public function testProcessFailsOnPassingClassToScalarTypedParameter()
		(new CheckTypeDeclarationsPass(true))->process($container);
		}

		public functiontestProcessSuccessOnPassingBadScalarType()
		public functionxtestProcessSuccessOnPassingBadScalarType()

Copy link

Member

stofJun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why skipping this test ?

Copy link

Author

GirgiasJun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I stole this from@nicolas-grekas WIP commit:nicolas-grekas@02e3881

Copy link

Member

nicolas-grekasJun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🙈

src/Symfony/Bundle/TwigBundle/Tests/Functional/AttributeExtensionTest.php

		@@ -51,7 +51,7 @@ public function registerContainerConfiguration(LoaderInterface $loader): void
		$container->register(StaticExtensionWithAttributes::class, StaticExtensionWithAttributes::class)
		->setAutoconfigured(true);
		$container->register(RuntimeExtensionWithAttributes::class, RuntimeExtensionWithAttributes::class)
		->setArguments(['prefix_'])
		->setArguments([true])

Copy link

Member

stofJun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

given thatRuntimeExtensionWithAttributes performs string concatenation with the prefix, the right fix is rather to change the constructor argument type in this RuntimeExtensionWithAttributes stub class.

stof reviewed

Jun 24, 2025

View reviewed changes

src/Symfony/Component/String/LazyString.php OutdatedShow resolvedHide resolved

Girgias added4 commits

June 25, 2025 14:23

intl false instead of null

85b52c5

Cast result of preg_match to bool

1f4dd46

Use bool as param type

d054d65

Cast bitwise op result to bool

65c324a

Girgias added6 commits

June 25, 2025 14:23

ErrorHandler::handleError() cast bitwise op to bool and use false ins…

b5a80da

…tead of 0

Yield empty string when no doc comment exists

1b7ac21

Set value to null if parse_url() fails badly and returns false

e2a60d3

Fix deprecation with PHPUnit's assertTrue

d13a389

Not sure what those asserts are doing however

Remove ZPP test

49f8c6d

There is no need to test false as this is a PHP engine behaviour to cast to empty string

file_get_contents() does not take a fopen mode parameter

a007578

Girgias force-pushed thebool-type-juggling branch fromb812e26 toa249be4Compare

June 25, 2025 13:23

Girgias added6 commits

June 25, 2025 22:11

Symfony\Component\String\LazyString::resolve() add explicit string cast

eefabd4

Fix string to true type juggling in test

90ffe94

Remove ZPP test

652b058

There is no need to test false as this is a PHP engine behaviour to cast to empty string

Use empty string instead of false

e9d4927

Explicit cast to bool from string

8351702

Definition::addMethodCall() arg 3 must be a bool v2

e2cdfd7

Girgias force-pushed thebool-type-juggling branch froma249be4 toe2cdfd7Compare

June 25, 2025 21:15

stof reviewed

Jun 27, 2025

View reviewed changes

src/Symfony/Component/DependencyInjection/Definition.php

		@@ -307,7 +307,7 @@ public function setMethodCalls(array $calls = []): static
		{
		$this->calls = [];
		foreach ($calls as $call) {
		$this->addMethodCall($call[0], $call[1], $call[2] ?? false);
		$this->addMethodCall($call[0], $call[1],isset($call[2]) && $call[2]);

Copy link

Member

stofJun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you have stack trace for the case triggering the deprecation here (without your patch) ? Maybe we have some tests not using the expected structure for the array, but it might be better to fix the root cause instead of a symptom (replacing the boolean type juggling in function context by a boolean type juggling in boolean operator context, which is not deprecated)

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

		$definition->addMethodCall(
		$call->getAttribute('method'),
		$this->getArgumentsAsPhp($call, 'argument', $file),
		(bool) XmlUtils::phpize($call->getAttribute('returns-clone'))

Copy link

Member

stofJun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

		@@ -589,7 +589,7 @@ private function parseDefinition(string $id, array\|string\|null $service, string
		} else {
		$method = $call[0];
		$args = $call[1] ?? [];
		$returnsClone = $call[2] ?? false;
		$returnsClone =isset($call[2]) && $call[2];

Copy link

Member

stofJun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same for those 2 cases. My expectation here is that we always expected a boolean Yaml value (or nothing) in such case, which would not trigger the deprecation with the existing code.
If we have some tests using non-boolean values, we might want to fix those tests instead (and maybe trigger a deprecation in Symfony for projects using a non-boolean value).

...mponent/Form/Tests/Extension/Core/DataTransformer/NumberToLocalizedStringTransformerTest.php

		@@ -65,7 +65,7 @@ public static function provideTransformations()
		public function testTransform($from, $to, $locale)
		{
		// Since we test against other locales, we need the full implementation
		IntlTestHelper::requireFullIntl($this,false);
		IntlTestHelper::requireFullIntl($this,null);