Magento code generation bug, resulting in mails not sending!

[NickvdMeij]

Preconditions

1. Have Magento 2.1.12 installed
2. PHP 7.0.26
3. MySQL 5.6

Steps to reproduce

1. Install https://github.com/Bogardo/Mailgun-Magento2 extension
2. Run setup:upgrade, di:compile and flush all cache
3. Configure the extension
4. Try and send a mail (order mail, invoice mail or any other mail)

Expected result

1. The mail should send correctly

Actual result

1. The mail does not send correctly, the following error is written to the logs:

2018-03-02 17:18:53] main.ERROR: Source class "\Nyholm\Psr7\Factory\Stream" for "Nyholm\Psr7\Factory\StreamFactory" generation does not exist. [] []

For some extra information see the following issues:
mailgun/mailgun-php#452
php-http/discovery#105

[orlangur]

There seems to be nothing to fix here from Magento side.

By convention, when class suffixed with Factory like Nyholm\Psr7\Factory\StreamFactory does not exist it is supposed to be autogenerated. As Nyholm\Psr7\Factory\Stream does not actually exist (as it is an optional dependency and not used in your scenario), autogeneration is not possible.

Thoughts?

[orlangur]

After a quick analysis,

38	7.2593	65405640	Mailgun\Mailgun->post( )	.../Mailgun.php:135
39	7.2593	65405640	Mailgun\Connection\RestClient->post( )	.../Mailgun.php:204
40	7.2594	65408760	Mailgun\Connection\RestClient->send( )	.../RestClient.php:179
41	7.2633	65435688	Http\Message\MultipartStream\MultipartStreamBuilder->__construct( )	.../RestClient.php:96
42	7.2693	65459280	Http\Discovery\StreamFactoryDiscovery::find( )	.../MultipartStreamBuilder.php:43
43	7.2693	65459280	Http\Discovery\ClassDiscovery::findOneByType( )	.../StreamFactoryDiscovery.php:25
44	7.2853	65562272	Http\Discovery\ClassDiscovery::evaluateCondition( )	.../ClassDiscovery.php:63
45	7.2882	65562648	Http\Discovery\ClassDiscovery::evaluateCondition( )	.../ClassDiscovery.php:174
46	7.2882	65562648	class_exists ( )	.../ClassDiscovery.php:164

the problem is that Nyholm\Psr7\Factory\StreamFactory is not a Magento-style autogenerated factory. In general it is not a good idea to use third party components directly how mentioned Mailgun module does it, especially when their behavior heavily rely on autoloading.

The right way to go is to wrap problematic third party library in Mailgun module as factory or adapter, like Magento Framework does it.

[NickvdMeij]

@orlangur By convention, when class suffixed with Factory like Nyholm\Psr7\Factory\StreamFactory does not exist it is supposed to be autogenerated.

What convention are you talking about? A Magento convention?

@orlangur The right way to go is to wrap problematic third party library in Mailgun module as factory or adapter, like Magento Framework does it.

If this is a common problem with most package, then might it just simply be the case that Magento has a faulty implementation?...
At the moment, we simply fixed it by locking the version of the package https://github.com/Bogardo/Mailgun-Magento2 (see https://github.com/MyCademy/Mailgun-Magento2) which works for us, but since this is a critical (silent) error, it's important that it gets fixed.

Can you imagine if problems like these arrises in (for example) the order process instead of just the mails? Then no orders will be correctly saved without any alarm bells ringing, resulting in many missed commerce.

There seems to be nothing to fix here from Magento side.

See the above, IMO the Magento generator should at least CHECK if the class it's trying to generate actually exists using the class_exists().

[orlangur]

Well,

Source class "\Nyholm\Psr7\Factory\Stream" for "Nyholm\Psr7\Factory\StreamFactory" generation does not exist.

is literally saying that the class does not exist.

By convention I mean that coding in Magento ecosystem you should know how classes like Nyholm\Psr7\Factory\StreamFactory are treated by Magento autoloader. This is described, for example, here: http://devdocs.magento.com/guides/v2.0/extension-dev-guide/factories.html#writing-factories

I don't see what can be fixed here on Magento side, as I described earlier https://github.com/Bogardo/Mailgun-Magento2 should not use non-Magento components like https://github.com/php-http/discovery directly, they should be wrapped in some custom factory or adapter in your module.

[NickvdMeij]

@orlangur I truly hope you are joking right? So, because you guys have made up your own convention, the whole PHP ecosystem has to rewrite their code to support it for Magento?... I can tell you up front, not gonna happen.

The bug is in the Magento autoloader BTW, if you guys build in a check if the class even exists before trying to make a factory out of it, the problem should never occur.

Furthermore, https://github.com/Bogardo/Mailgun-Magento2 isn't using php-http/discovery directly as you can see in the composer.json. It is using the official mailgun/mailgun-php package, which on his turn depends on php-http/discovery.

Last but not least,

Well,

Source class "\Nyholm\Psr7\Factory\Stream" for "Nyholm\Psr7\Factory\StreamFactory" generation does not exist.

is literally saying that the class does not exist.

It is already that smart that it knows it doesn't exist, then why on earth is it trying to make a factory out of it in the first place?!

[orlangur]

It is using the official mailgun/mailgun-php package

Then this package have to be wrapped in a proper class. The whole point is that client code should not depend on third party components directly.

why on earth is it trying to make a factory out of it in the first place?!

That's how factory autogeneration works, please get familiar with documentation provided by me.

The bug is in the Magento autoloader BTW

Please update issue description with expected result in terms of Magento autoloader behavior.

Current behavior: when there is no class AnyVendor\AnyPackage\AnyClassFactory, it is generated out of AnyVendor\AnyPackage\AnyClass. If there is no such class, exception must be thrown as usually it happens when you mistyped class name.

[NickvdMeij]

That's how factory autogeneration works, please get familiar with documentation provided by me.

Yeah, that's how Magento does factory autogeneration, and it's incorrect and leads to unexpected bugs. That's why I opened this issue. Simply saying "oh don't use the word Factory in your class names" is a wrong way of doing autogeneration IMO.

If there is no such class, exception must be thrown as usually it happens when you mistyped class name.

Like said earlier, there is no exception that is being thrown, and if there is an exception thrown it get caught. However, in the adminhtml, you get a 200 OK when you send the mail, except the mail does not get sent (like I said earlier, its a silent error, hence the danger of leaving the autogenerator as-is)

Then this package have to be wrapped in a proper class. The whole point is that client code should not depend on third party components directly.

Well if this is the case, then good luck wrapping all PHP packages with a class that has the word Factory that exist in an additional layer of abstraction... for basically nothing...

@orlangur t.b.h. do what you want with this issue, we got our application working again but be prepared to encounter more and more bugs like this to appear in the future until this is fixed. I only made this issue to inform you guys what is going on, already spend 2 days debugging trying to figure out what is wrong so I have no need for an endless "yes/no" discussion.

[orlangur]

Well, it would be much faster to just read the manual http://devdocs.magento.com/guides/v2.0/extension-dev-guide/factories.html#writing-factories prior to writing Magento code.

leads to unexpected bugs

It is quite expectable to implement buggy software if you don't follow the best practices. Third party components are wrapped in classes for a reason and not just because Magento likes to have as much classes as possible. You faced exactly with the problem which is solved by such approach: upgradeability. Another problem solved is replaceability, you may replace third party component preserving just an interface.

However, in the adminhtml, you get a 200 OK when you send the mail, except the mail does not get sent (like I said earlier, its a silent error, hence the danger of leaving the autogenerator as-is)

This is clearly a bug in https://github.com/Bogardo/Mailgun-Magento2 and needs to be fixed. Emails are sent asynchronously in recommended setup thus you cannot see error in UI but of course logs should be monitored for errors.

@orlangur t.b.h. do what you want with this issue

Thanks for the quick feedback, closing this issue as per discussion. Feel free to reach me out anytime later if you come out with the way autogeneration may be improved for this case without breaking other important scenarios.

[Nyholm]

I have briefly read the discussion and I've read the link @orlangur provided. If I understand this Magento conversion properly, then all code will break if I write:

$bool = class_exists('FooFactory');

Is that really the intention?

[orlangur]

@Nyholm yes, that's how https://en.wikipedia.org/wiki/Fail-fast is supposed to work. Developer is quickly notified that he is referring to nonexistent Foo class.

As I understood the problem, some kind of feature detection is implemented with class_exists('SomeOfTheFactory') in a third-party library. In Magento world instead of such magic there would be a concrete implementation preference in di.xml. We prefer explicit over implicit.

So, this particular logic need to be wrapped in order to make library compatible with Magento autogeneration.

[Nyholm]

@Nyholm yes, that's how https://en.wikipedia.org/wiki/Fail-fast is supposed to work.

No. This is were you are wrong. It should not break anything. It should return false. Check the manual: http://php.net/manual/en/function.class-exists.php

As I understood the problem, some kind of feature detection is implemented with class_exists('SomeOfTheFactory') in a third-party library. In Magento world instead of such magic there would be a concrete implementation preference in di.xml. We prefer explicit over implicit.

I understand that Magento has some sweet magic. But this is a PHP library. It does not care for any specific conventions etc etc in Symfony, Magento, Zend or any framework.

---

If one would like to use a third party PHP library that contains the line $bool = class_exists('FooFactory');, how could one make sure no exception is thrown? Could that developer make some changes to the configuration of his/her application? Modifying the third party library is of course not an option.

[magento-engcom-team]

@NickvdMeij, thank you for your report.
This seems to be correct Magento behavior. Please refer to the Community Forums or the Magento Stack Exchange site for advice or general discussion about this.
Otherwise you may submit Pull Request with the suggested changes.

[orlangur]

It should not break anything. It should return false. Check the manual: http://php.net/manual/en/function.class-exists.php

Nope, see for example https://stackoverflow.com/a/1589227/8640867

I understand that Magento has some sweet magic. But this is a PHP library. It does not care for any specific conventions etc etc in Symfony, Magento, Zend or any framework.

The magic comes from a library calling class_exists for nonexisting classes. That's exactly why you should not use low level third party libraries (ideally - any of them) in a client code.

If one would like to use a third party PHP library that contains the line $bool = class_exists('FooFactory');, how could one make sure no exception is thrown? Could that developer make some changes to the configuration of his/her application? Modifying the third party library is of course not an option.

That's exactly why it is not safe to bring third party libraries into your client code without proper testing. Library should be either wrapped in your own classes or configured the way it plays well within Magento. For example, if https://github.com/php-http/discovery was configured in such a way that it does not perform such class_exists operations.

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.

[Nyholm]

It should not break anything. It should return false. Check the manual: http://php.net/manual/en/function.class-exists.php

Nope, see for example https://stackoverflow.com/a/1589227/8640867

... Im not sure what you are referring to.

The magic comes from a library calling class_exists for nonexisting classes.

There is not magic with calling class_exists. That function should check if a class exists or not...

That's exactly why you should not use low level third party libraries (ideally - any of them) in a client code.

Wow, this seams a bit hostile to me. Telling people to not use other libraries is not the way you create "A Flexible, Open Source Commerce Platform" as you write in the heading of the website.

---

I hear that you are open for improvements. That is awesome. Im afraid I know too little Magento to make any specific suggestions. I added a PR (#14085) with a unit test.

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.

[orlangur]

That function should check if a class exists or not...

It triggers autoloading in some cases which obviously can throw exceptions.

Wow, this seams a bit hostile to me. Telling people to not use other libraries is not the way you create "A Flexible, Open Source Commerce Platform" as you write in the heading of the website.

That's not what I was saying. You should not use external classes directly in controllers/models/blocks etc but properly encapsulate them. So that you can upgrade or replace third party components easily afterwards.

This is true even for PHP functions, like https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/Serialize/Serializer/Json.php.

The fix is as simple as encapsulating code doing risky things

echo "Starting\n\n";
$messageFactory = \Http\Discovery\MessageFactoryDiscovery::find();
if (null !== $messageFactory) {
    echo "Class exists!";
} else {
    echo "Fail";
}

in your own class so that it does not trigger autoloading with code generation.

I would suggest not to create classes when they are being autoloaded.

This would mean to remove proxies/factories autogeneration on demand feature which is pretty useful in developer mode.

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.

This is exactly what setup:di:compile does, in production mode all proxies and factories are pregenerated. But, trying to access factory for nonexistent class will fail anyway.

[orlangur]

@Nyholm,

Discovery is simply a convenience wrapper to statically access clients and factories for when Dependency Injection is not an option. Discovery is useful in libraries that want to offer zero-configuration services relying on the virtual packages.

Static calls and no way to configure component behavior is not a good choice when you implement Magento module. It would mean that you make your extension not extendable for others.

Not just Magento core is quite flexible but also third party modules should be implemented keeping totally custom implementation in mind. So that you can install third party module and then slightly change its behavior with a custom implementation.