Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ThemeCollector Symfony 7 compatibility #134

Merged

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Jul 15, 2024

PHP Fatal error: Type of Sylius\Bundle\ThemeBundle\Collector\ThemeCollector::$data must be Symfony\Component\VarDumper\Cloner\Data|array (as in class Symfony\Component\HttpKernel\DataCollector\DataCollector) in /home/runner/work/SyliusThemeBundle/SyliusThemeBundle/src/Collector/ThemeCollector.php on line 24

I added a very simple test case which just init the ThemeCollector to catch this.

@alexander-schranz alexander-schranz marked this pull request as ready for review July 15, 2024 13:29
@@ -60,7 +55,7 @@ public function getUsedTheme(): ?ThemeInterface
}

/**
* @return array|ThemeInterface[]
* @return ThemeInterface[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see phpdoc above

*
* @psalm-suppress NonInvariantDocblockPropertyType
*/
protected $data;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable is defined by DataCollector itself. With beginning of Symfony 7 it has a Union Type here: https://github.com/symfony/symfony/blob/1a16ebc32598faada074e0af12a6a698d2964a5e/src/Symfony/Component/HttpKernel/DataCollector/DataCollector.php#L31

For BC reasons best is to move this variable to PHPDoc definition. So we can support Symfony 7 and previous versions of the parent collector class.

Comment on lines +84 to +85
"tests/Application/bin/console lint:container --env dev",
"tests/Application/bin/console lint:container --env prod"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stumbled now over that you also have a working console the lint:container would also catch such errors. I recommend always lint for dev and prod container as they may have different set of services.

src/Collector/ThemeCollector.php Outdated Show resolved Hide resolved
Co-authored-by: Grzegorz Sadowski <[email protected]>
@GSadee GSadee merged commit df263ac into Sylius:2.4 Jul 18, 2024
6 checks passed
@GSadee
Copy link
Member

GSadee commented Jul 18, 2024

Thank you, Alexander! 🥇

@alexander-schranz alexander-schranz deleted the bugfix/symfony-7-support-theme-collector branch July 18, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants