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

Prevent trying to autoload Throwable #2402

Closed
wants to merge 1 commit into from
Closed

Prevent trying to autoload Throwable #2402

wants to merge 1 commit into from

Conversation

kronthto
Copy link

PR for comment in #2273 (comment) .

It will prevent interface_exists from attempting to autoload the SPL interface, which is bound to fail.

With an autoloader implementation that doesn't properly handle not finding a class it can cause warnings (had this issue with the Magento1 autoloader).

The only risk I can think of is very abstract, as I don't think it's common that PHP5 environments would provide a polyfill for \Throwable in autoloaded code.

I didn't figure out how to write a relevant test for this change.

@GrahamCampbell
Copy link
Member

Hmmm. I'd say the autoloader should not be crashing the application like that. Warning for classes that don't exist is silly because testing if a class exists is so common.

@riccardomessineo
Copy link

This is exactly the PR I was going to submit. Had the same issue with Yii.

@sagikazarmark
Copy link
Member

The only risk I can think of is very abstract, as I don't think it's common that PHP5 environments would provide a polyfill for \Throwable in autoloaded code.

Are there any references supporting that claim? I'm not saying it's wrong, but I can't say from the top of my head that they don't.

@kronthto
Copy link
Author

kronthto commented Dec 8, 2019

symfony/polyfill-php70 for one doesn't do it, the \Error stub extends \Exception and they don't provide a \Throwable.

That's of course not proving some don't.

@Nyholm Nyholm changed the base branch from master to 6.5 December 8, 2019 16:24
@sagikazarmark
Copy link
Member

After thinking this through, I agree with @GrahamCampbell, an autoloader should not fail when a class does not exist.

My primary concern is breaking polyfills that do provide a Throwable stub.

An easy fix is providing a stub in an environment where the autoloader can't handle the situation, but I think it should be an edge case.

@Nyholm
Copy link
Member

Nyholm commented Dec 21, 2019

According to the PSR4 autoloader specification

  1. Autoloader implementations MUST NOT throw exceptions, MUST NOT raise errors of any level, and SHOULD NOT return a value.

I know it sucks that Magento decided to build an autoloader that does not comply with PSR-4. However, I know that newer versions of Magento will fix this issue. =)

I will close this issue since it is not related to Guzzle. Mark also provided a workaround.

@Nyholm Nyholm closed this Dec 21, 2019
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.

None yet

5 participants