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 #32396 #32430

Closed
wants to merge 1 commit into from
Closed

Fix #32396 #32430

wants to merge 1 commit into from

Conversation

mikeSimonson
Copy link
Contributor

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32396
License MIT
Doc PR

The src/Symfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php implements
the FormTypeGuesserInterface and that interface is only defined in the
symfony/form component.
By implementing that interface that code depends on it not only for test but
also in production use.

We discovered it because we have an api which doesn't require symfony form
who blew up with the update to php 7.2.20.
The release of php 7.2.20 fixed a bug that was allowing the implementation
of interface that are not available.

The src/Symfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php implements
the FormTypeGuesserInterface and that interface is only defined in the
symfony/form component.
By implementing that interface that code depends on it not only for test but
also in production use.

We discovered it because we have an api which doesn't require symfony form
who blew up with the update to php 7.2.20.
The release of php 7.2.20 fixed a bug that was allowing the implementation
of interface that are not available.
@teohhanhui
Copy link
Contributor

This is not the right fix. DoctrineOrmTypeGuesser is only useful when using the Form component, that's why it's an optional dependency.

@derrabus
Copy link
Member

derrabus commented Jul 8, 2019

Plus the PR would only heal a symptom and does not address the real problem. There are enough other constellations that will break because of this bug and adding unnecessary dependencies can't be the solution to all of them.

@mikeSimonson
Copy link
Contributor Author

@teohhanhui As long as that class in that package require the interface that interface is required all the time not only when you use the form component. That dependency has nothing optional right now.

@mikeSimonson
Copy link
Contributor Author

@derrabus How can you see that dependency as optional. The code is using it thus it's not optional.

@teohhanhui
Copy link
Contributor

teohhanhui commented Jul 8, 2019

The real problem is php fixed it the wrong way, inconsistent with how optional dependencies are actually used in the real world. Or perhaps there's still a way to catch the fatal error. I don't know. But this is definitely not the right fix.

@mikeSimonson
Copy link
Contributor Author

mikeSimonson commented Jul 8, 2019

@teohhanhui How do you want php to validate whether you correctly implement the interface if you don't give it the interface. Is it just supposed to assume it's correct. Because if that's what you believe, great you just made interface utterly useless.

PHP corrected it the right way

@teohhanhui
Copy link
Contributor

@mikeSimonson Please refer back to the discussion in https://bugs.php.net/bug.php?id=76980

@nikic said:

What's the expected behavior here? That Foo should not be defined at all since the interface couldn't be loaded?

Yes, that would be the expected behavior.

I'm not sure if it's a PHP bug or a Symfony bug at this point, but generally speaking, using optional dependencies is a very widespread practice in many Composer packages.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 10, 2019
@nicolas-grekas
Copy link
Member

Closing as explained in the linked issue. The root cause is discussed in #32395

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

5 participants