Skip to content

[Runtime] Fix class validation of composer "extra.runtime.class" #43376

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2021
Merged

[Runtime] Fix class validation of composer "extra.runtime.class" #43376

merged 1 commit into from
Oct 14, 2021

Conversation

piku235
Copy link
Contributor

@piku235 piku235 commented Oct 8, 2021

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

This PR fixes a bug of validating a runtime class from the composer param extra.runtime.class. When provided, it always fails. Taking as an example the Runtime\Swoole\Runtime of the runtime/swoole package, when executing the composer dump-autoload, it ends with the following exception, even though the runtime/swoole package was installed.

 [InvalidArgumentException]
 Class "Runtime\Swoole\Runtime" listed under "extra.runtime.class" in your composer.json file not found.

When a composer plugin is executed, the composer autoloader for the project is not registered, thereby any reference to a class from installed packages won't work.
The fix drops this check to avoid it. I don't think it's worth checking here if a runtime class is valid or not. For instance, anything that comes from the $_SERVER['APP_RUNTIME'] is accepted, nothing is validated.

@carsonbot carsonbot added this to the 5.3 milestone Oct 8, 2021
@piku235 piku235 changed the title [Runtime] Drop class validation of composer "extra.runtime.class" [Runtime] Fix class validation of composer "extra.runtime.class" Oct 8, 2021
@Ahummeling
Copy link
Contributor

Ahummeling commented Oct 12, 2021 •
edited
Loading

Perhaps this issue can be resolved without omitting the check entirely. Could it be the case that the class_exists($runtimeClass) call is responsible for the failed autoload? If so, does the issue still occur when passing false as second parameter to prevent calling autoload by default?

@piku235
Copy link
Contributor Author

piku235 commented Oct 13, 2021 •
edited
Loading

@Ahummelin I'd also like to keep it, but because of how things internally work for composer plugins it seems hard to get and therefore not worth it. The issue is more about the is_subclass_of() call in the if statement. When a class does not exist, it tries to reach registered autoloaders, but as the most important one - responsible for project dependencies is not registered when a composer plugin is executed, it returns always false.

Ahummeling
Ahummeling approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@Ahummeling Ahummeling left a comment

Choose a reason for hiding this comment

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

Yeah I see what you mean now, going over the doc for is_subclass_of, seems that the autoloader can be disabled, but then you'd need to get an instance of the class, which was what we're trying to get around in the first place...

With that said, I think the PR looks good

@nicolas-grekas
Copy link
Member

Thank you @piku235.

@nicolas-grekas nicolas-grekas merged commit 166ca8d into symfony:5.3 Oct 14, 2021
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 28, 2021
Closed
@fabpot fabpot mentioned this pull request Oct 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof approved these changes

@Ahummeling Ahummeling Ahummeling approved these changes

Assignees
No one assigned
Labels
Bug Runtime Status: Reviewed
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
5 participants
@piku235 @Ahummeling @nicolas-grekas @stof @carsonbot