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

The autoloader throws an exception on class_exists. #14085

Closed
wants to merge 1 commit into from

Conversation

Nyholm
Copy link

@Nyholm Nyholm commented Mar 13, 2018

Description

The autoloader throws an exception on class_exists. That is clearly an unexpected behavior according to the documentation: http://php.net/manual/en/function.class-exists.php

This will cause issues when including third party libraries that is not written for Magento.

Fixed Issues (if relevant)

  1. Magento code generation bug, resulting in mails not sending! #14074: Magento code generation bug, resulting in mails not sending!

Manual testing scenarios

Run class_exists('FooFactory'). It should return false but an error is thrown instead.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

FYI @NickvdMeij

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 13, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@orlangur
Copy link
Contributor

We cannot process PR with broken tests. Prior to a full code review please make sure all Travis CI builds are green.

@Nyholm
Copy link
Author

Nyholm commented Mar 13, 2018

This PR is not meant to fix anything. It is meant to show that the code has a bug.

Someone with greater knowledge of Magento internals should use this PR/commit/test and add a fix to the problem.

@orlangur
Copy link
Contributor

@Nyholm thank you for your endeavours but factories autogeneration is a crucial Magento feature and it cannot be removed just because it causes troubles in some random library.

Instead of hacking Magento core recommended approach in such situation is to implement compatibility layer.

@Nyholm
Copy link
Author

Nyholm commented Mar 13, 2018

One do not have to remove code just because they it has a bug in it. One could simply try to improve in instead.

Thank you for taking the time to read and answer my posts.

@orlangur
Copy link
Contributor

@Nyholm, from another thread

I'm open for any improvements into current Magento behavior. How you think autoloder can determine whether factory should be generated when FooFactory is requested? Some flags to determine whether code is Magento-style or not does not seems to be a viable option.

One suggestion was

I would suggest not to create classes when they are being autoloaded. The creation of classes should happen when the project is built (ie when the cache is warming up). That would also make it easier to deploy Magento on read-only filesystems.

having a huge Magento expertise I simply don't see what can be changed to keep both scenarios workable.

I don't ask you for a code, if you ever come up with a conceptual idea just share it. For the reference, here is the current implementation: https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/Code/Generator.php#L91

@kandy kandy changed the title Added failing tests to show error in autoloader The autoloader throws an exception on class_exists. Dec 6, 2018
@kandy kandy reopened this Dec 6, 2018
@Vinai
Copy link
Contributor

Vinai commented Dec 6, 2018

An autoloader that throws an exception is not PSR-4 compliant:

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

@orlangur
Copy link
Contributor

orlangur commented Dec 6, 2018

Thanks @Vinai for pointing that out :)

@Vinai @kandy what do you propose to do with this PR? It does not include any solution, to me it makes more sense to reopen a corresponding issue or report a new one describing what is not so good in current core implementation.

@sidolov
Copy link
Contributor

sidolov commented Jan 29, 2019

Hi @Nyholm , @orlangur , I'm closing this PR because it does not contain the solution for the described issue. The new issue was created with all needed information #20773. Feel free to add additional info if I missed something.
Thank you!

@sidolov sidolov closed this Jan 29, 2019
@ghost
Copy link

ghost commented Jan 29, 2019

Hi @Nyholm, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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.

6 participants