Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[String] Check if function exists before declaring it #40203

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

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Feb 16, 2021

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

If you installed a command line tool like psalm with composer and then try to run it on a project that included the String component you will get an error like:

Fatal error: Cannot redeclare Symfony\Component\String\u() (previously declared in /Workspace/symfony/src/Symfony/Component/String/Resources/functions.php:14) in /user/.composer/vendor/symfony/string/Resources/functions.php on line 14

That is because we are loading two installations of the string component.

src/Symfony/Component/String/Resources/functions.php Outdated Show resolved Hide resolved
@Nyholm
Copy link
Member Author

Nyholm commented Feb 16, 2021

I've updated the code to use ::class, I've also fixed the same issue in Translation component

@nicolas-grekas
Copy link
Member

Thank you @Nyholm.

@nicolas-grekas nicolas-grekas merged commit 7dcf156 into symfony:5.2 Feb 16, 2021
@Nyholm
Copy link
Member Author

Nyholm commented Feb 16, 2021

Thank you for merging.

@Nyholm Nyholm deleted the string-function-exists branch February 16, 2021 10:21
@fabpot fabpot mentioned this pull request Mar 4, 2021
@weirdan
Copy link

weirdan commented Mar 6, 2021

If you installed a command line tool like psalm
[...]
I've updated the code to use ::class

Ironically, this broke Psalm tests: https://github.com/vimeo/psalm/runs/2037304361#step:7:42

I would suggest to replace

if (!\function_exists(u::class)) {

with

if (!\function_exists(__NAMESPACE__ . '\\u')) {

@Nyholm
Copy link
Member Author

Nyholm commented Mar 6, 2021

The current implementation is valid PHP code. I’m not sure what the benefits are to implement to your suggestion.

I don’t want to be disrespectful, but this is a problem psalm should fix, isn’t it?

@weirdan
Copy link

weirdan commented Mar 6, 2021

I’m not sure what the benefits are to implement to your suggestion.

Clearer code, mostly. I had this little 'huh?' moment looking at it, thinking 'What are those weirdly named classes? And why are they used in function_exists()? Oh, those are not classes...'

this is a problem psalm should fix, isn’t it?

Perhaps. Hardly a high-priority issue though, as in practice it will only happen when people run Psalm with badly written custom autoloader.

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.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.