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

Moves common code to all auto-generated Interceptor classes into a trait #1156

Closed
wants to merge 0 commits into from

Conversation

fooman
Copy link
Contributor

@fooman fooman commented Mar 31, 2015

This implements using traits for the interceptor code as discussed in issue #975

I have run a few performance tests using https://github.com/magento/magento2/blob/develop/dev/tests/performance/benchmark.jmx

Below are some figures copied from the Summary Reports

interceptors-as-traits-rc starting with deleted caches + var/generation 
TOTAL   32  740 1   6485    1577.6537563736854  0.0 1.322587311427981   17.65311789625956   13667.75

0.74.0-beta2 starting with deleted caches + var/generation
TOTAL   32  776 1   6581    1706.4700590693058  0.0 1.2645722189290656  16.87409695465323   13663.96875


interceptors-as-traits-rc warmed caches
TOTAL   32  503 1   6446    1310.2442009835179  0.0 1.9126172972326818  25.52119521770964   13663.84375

0.74.0-beta2 warmed caches
TOTAL   32  508 0   6571    1330.0127022272898  0.0 1.9099916437865583  25.486334420138473  13663.9375

which to me looks like no performance impact good or bad.

@fooman
Copy link
Contributor Author

fooman commented Mar 31, 2015

The following tests failed on travis

unit:

  1. Magento\Payment\Test\Unit\Block\Info\CcTest::testGetCcExpDate with data set #0 (2, 2015)

Failed asserting that '03' matches expected 2.

/home/travis/build/magento/magento2/app/code/Magento/Payment/Test/Unit/Block/Info/CcTest.php:147

which also fails on other pull requests (https://travis-ci.org/magento/magento2/jobs/56015518) so I don't think is related to my changes.

integration:

  1. Magento\Reports\Model\Resource\Report\Product\Viewed\CollectionTest::testTableSelection with data set __DIR__ Typo fixed #3 ('month', 'report_viewed_product_aggregated_yearly', NULL, '2015-03-31')

Failed asserting that an array has the key 'travis_report_viewed_product_aggregated_yearly'.

/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Reports/Model/Resource/Report/Product/Viewed/CollectionTest.php:68

again I think this is unrelated. The above is a single failure in an 18 set dataset - if this was related to the interceptor code I am fairly sure it would fail on all.

@otoolec
Copy link
Contributor

otoolec commented Apr 7, 2015

Looks good to me, will get this in using internal ticket MAGETWO-35920. Thanks for the contribution!

@vpelipenko vpelipenko added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Apr 21, 2015
@otoolec
Copy link
Contributor

otoolec commented Apr 28, 2015

Sorry for the delay on this one. I noticed that Zend\Code\Generator\ClassGenerator added support for traits in version 2.4 so I'm holding off on accepting this PR until we can update the version of ZF2 used by M2. That will allow us to avoid copy pasting code in Magento\Framework\Code\Generator\ClassGenerator.

@fooman
Copy link
Contributor Author

fooman commented Apr 28, 2015

@otoolec Thanks for the update - sounds good.

@otoolec
Copy link
Contributor

otoolec commented May 19, 2015

@fooman Magento 2 is now using version 2.4 of ZF2 components. This means there shouldn't be a need to copy Zend code into Magento. Would you mind updating the pull request to reflect that simplification?

@fooman
Copy link
Contributor Author

fooman commented May 25, 2015

The travis integration_integrity build seems to have had some issue Travis side:

$ phpenv global 5.5

/home/travis/build.sh: line 41: phpenv: command not found

The command "phpenv global 5.5" failed and exited with 127 during .

the remaining tests look good.

@otoolec
Copy link
Contributor

otoolec commented May 28, 2015

Thanks @fooman for making the updates. I'll work on integrating this into our code and running the internal test-suite. Will let you know if I hit any snags.

@fooman
Copy link
Contributor Author

fooman commented Jun 1, 2015

Looks like the latest beta-11 release had some merge conflicts (Fixed an issue where interceptors were Generated with Invalid __wakeup()). I have resolved those.

@fooman
Copy link
Contributor Author

fooman commented Jun 2, 2015

Failures are the same as the beta-11 release.

@vpelipenko
Copy link
Contributor

Yes, we've also faced with some failed tests on Travis. We are working on it now.

magento-team pushed a commit that referenced this pull request Jun 19, 2015
…ceptor classes into a trait #1156

Add some doc-blocks and fix some static test failures
magento-team pushed a commit that referenced this pull request Jun 19, 2015
…ceptor classes into a trait #1156

Add interface for Interceptor so Chain can reference it.
@joanhe
Copy link
Contributor

joanhe commented Aug 6, 2015

@fooman Could you take a look and resolve the conflicts?

@fooman
Copy link
Contributor Author

fooman commented Aug 8, 2015

@joanhe to be honest I am currently not sure how this PR was treated. The contributed code is already merged in - see here: https://github.com/magento/magento2/blame/develop/lib/internal/Magento/Framework/Interception/Interceptor.php

Maybe @otoolec can clarify but I think this has already been merged and this can get closed.

@otoolec
Copy link
Contributor

otoolec commented Aug 11, 2015

I suspect the conflicts are related to b15f387 and should be easy to resolve. I would suggest dropping that commit from this PR and seeing if that resolves the conflict.

@fooman fooman closed this Aug 12, 2015
@fooman fooman force-pushed the interceptor-as-traits-rc branch from b15f387 to d596078 Compare August 12, 2015 02:54
@fooman
Copy link
Contributor Author

fooman commented Aug 12, 2015

thanks @otoolec - dropping the merge seems indeed to have resolved it and it already shows as merged and closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: needs update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants