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

 add return type hints to EntityFactory #52234

Merged
merged 1 commit into from Oct 22, 2023

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Oct 22, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

see https://github.com/EasyCorp/EasyAdminBundle/actions/runs/6603120076/job/17935909075

@carsonbot carsonbot added this to the 6.4 milestone Oct 22, 2023
@xabbuh xabbuh changed the title block the doctrine bridge from being used with SecurityBundle 7 [DoctrineBridge] block the bridge from being used with SecurityBundle 7 Oct 22, 2023
@wouterj
Copy link
Member

wouterj commented Oct 22, 2023

An alternative would be to add the native return type and document the BC break. We've done that for multiple other method implementations as well: #50890 Given EntityFactory is a DI factory class, the likelihood of anyone being hit by this BC break seems rather slim.

@nicolas-grekas
Copy link
Member

I like @wouterj's proposal!

@xabbuh xabbuh force-pushed the doctrine-bridge-conflict-security-bundle branch from f7ec47e to b778bc9 Compare October 22, 2023 14:19
@derrabus
Copy link
Member

Yes, let's do what @wouterj proposes. If someone complains about the BC break, we could still make the return type conditional using a trait, but I doubt that someone extends this class.

While we're at it, should we make EntityFactory final in 7.0? There's really no point in extending it: It only exposes the public methods from UserProviderFactoryInterface. If someone wants to extend the functionality of that class, a decorator would be the better way to do that.

@xabbuh xabbuh changed the title [DoctrineBridge] block the bridge from being used with SecurityBundle 7 [DoctrineBridge] add return type hints to EntityFactory Oct 22, 2023
@xabbuh xabbuh force-pushed the doctrine-bridge-conflict-security-bundle branch from b778bc9 to 3ceed63 Compare October 22, 2023 14:23
@xabbuh
Copy link
Member Author

xabbuh commented Oct 22, 2023

I have added the return types and also marked the class as final.

@carsonbot carsonbot changed the title [DoctrineBridge] add return type hints to EntityFactory  add return type hints to EntityFactory Oct 22, 2023
@smnandre
Copy link
Contributor

Is this the same problem there ?

PHP Fatal error: Declaration of Symfony\WebpackEncoreBundle\CacheWarmer\EntrypointCacheWarmer::doWarmUp($cacheDir, Symfony\Component\Cache\Adapter\ArrayAdapter $arrayAdapter): bool must be compatible with Symfony\Bundle\FrameworkBundle\CacheWarmer\AbstractPhpFileCacheWarmer::doWarmUp(string $cacheDir, Symfony\Component\Cache\Adapter\ArrayAdapter $arrayAdapter, ?string $buildDir = null): bool in /home/runner/work/ux/ux/src/Turbo/vendor/symfony/webpack-encore-bundle/src/CacheWarmer/EntrypointCacheWarmer.php on line 27

symfony/ux#1223

If so, could you explain what are the steps to fix ? (i must admit i'm not yet fluent in "multi-php multi-version multi-bundle" troubles 😅

@wouterj
Copy link
Member

wouterj commented Oct 22, 2023

@smnandre that's a different issue: the first parameter $cacheDir must get the string type in the WebpackEncoreBundle. Adding types to parameters can be done safely (it's not a BC break), as a child can have a less specific parameter type than its parent).

@smnandre
Copy link
Contributor

Thank you @wouterj !

@fabpot
Copy link
Member

fabpot commented Oct 22, 2023

Thank you @xabbuh.

@fabpot fabpot merged commit fafbbfa into symfony:6.4 Oct 22, 2023
7 of 9 checks passed
@xabbuh xabbuh deleted the doctrine-bridge-conflict-security-bundle branch October 22, 2023 15:17
@derrabus
Copy link
Member

derrabus commented Oct 22, 2023

the first parameter $cacheDir must get the string type in the WebpackEncoreBundle

Actually, the issue is the new and optional third parameter. But, as @wouterj said, this is a different and unrelated problem. If you don't know how to solve it, please open a new issue or a dicussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants