[Question] Have traits been considered for the Interceptor classes?

[fooman]

The auto generated Interceptor classes currently duplicate the code when they get generated. Since Magento is now 5.4+ have traits been considered to bring the common functionality into the generated classes?

It would also make this code
https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Interception/Code/Generator/Interceptor.php#L170
easier to discover, read and maintain.

[antonkril]

Generated code doesn't lead to maintainance problems because its logic is in single place. Traits would not bring any value but would just add one more file to interceptor code.

[antonkril]

Does this answer your question? Can we close the issue?

[fooman]

@antonkril I think you might have misunderstood my question.

My comment in regards to maintainability, readability and so forth is about the generating code not the generated code. I haven't yet been able to run the performance tests however this is what I am referring to:

For example this code here
https://github.com/fooman/magento2/compare/interceptor-as-trait#diff-7fea50965bcc6aeb72cc4f333e0f9912R1 is easier to read than this https://github.com/fooman/magento2/compare/interceptor-as-trait#diff-4c790e1d84b8acca1907cade0a2ae3d7L122

The code can also be subjected to code analysis plus the trait can be discovered from the generated code.

[SchumacherFM]

The trait approach is more developer friendly as you can directly hack in the trait and try things out.

👍 for @fooman

[Flyingmana]

If I understand it correctly, its about the shared code the generator adds to many classes, which is always the same.
Not knowing much of the code here, but does it change behavior if someone has an own __sleep method defined in a class?

[fooman]

@Flyingmana the Interceptor classes, living under var/generated, are autogenerated here https://github.com/fooman/magento2/blob/master/lib/internal/Magento/Framework/Interception/Code/Generator/Interceptor.php#L205

The generating code copies over the public methods from your plugin code but excludes __sleep,__wakeup and __clone, so with current behaviour you would not be able to have your own implementations of these.

General trait behaviour would be The precedence order is that methods from the current class override Trait methods, which in turn override methods from the base class.

[Marko-M]

👍 for trait approach. Traits are there, they make code more readable/maintainable therefore we should use it.

[antonkril]

@fooman we considered template approach for code generation. With templates Interceptor code would be in separate file and generator would be simpler. But we considered this low priority task because 3PDs are not supposed to work with this code anyway.

Traits are for code reuse, code can be reused by aggregation but we kept that code in interceptor class to make it easier to debug interceptor code (3PD will rarely/never encounter generator code, but will encounter Interceptor code quite often during debugging).

@Flyingmana Intercetpor calls parent::__sleep() so it will be executed.

[fooman]

@antonkril In reply to this comment:

But we considered this low priority task because 3PDs are not supposed to work with this code anyway.

3PD = third party developers?

While it might not be envisaged that this code will be changed by third parties Interceptors are one of the major new features of Magento 2 and developers would/should be working all the time with this code.

As this is a low priority on your end: If I finish the pull request and provided there is no major impact on performance what is the chance of this being accepted?

[antonkril]

3PDs (yes, third party developers) will be working with Interceptors frequently (so we try to keep them simple) but not with their generation.

Refactoring of generation is not high priority but Interceptors should stay simple. So in my opinion any pull request that spreads Interceptor code should only be considered if it has performance improvement.

But this question is not important (two options are not that different and I don't think there will be any performance difference) and trait version seems to have some support in community so we can consider it.

[antonkril]

Update: Trait implementation is considered as a possible way of simplifying debugging of intercepted code

[vpelipenko]

Guys, can we consider this question answered?

[Flyingmana]

from my point of view: yes

[magento-engcom-team]

Hi @fooman. Thank you for your report.
The issue has been fixed in magento/graphql-ce#997 by @atwixfirster in 2.3-develop branch
Related commit(s):

• 493d7441b1644647dcdce2ab94e4c13d8bed2fb0

The fix will be available with the upcoming 2.3.4 release.