Skip to content

Include support for banner/ajax/load controller (enterprise specific) #13

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidalger
Copy link

Have a Magento Commerce 2.2 site which I just rolled out a modified version of this extension on a short while ago. Enterprise has a banner/ajax/load call which similarly reads from session, but never writes, so the changes here are adding support for that.

On my local test site with a sleep(5) added to exaggerate requests to both banner/ajax/load and customer/section/load for easily testing the lock release I get the following results:

Before installation (click to expand image):
Screen-Shot-2019-11-13-13-01-48_48-before-with-sleep-5-B1Vq1J0EZFTpCojYcK88RlHagAqOu9VdXJWkFZjfJz4RyXMYlmZ6BKrklv5l1u8y8d6U4d3YwtqvmnTXX2MQv08cRbZn7ox82Moe

After modified module is installed (click to expand image):
Screen-Shot-2019-11-13-13-01-48_48-after-with-sleep-5-3rOb84oBWaNDrq4mKbw4zvhTMbTvdImdzs06gME6diSZZJGV9xImJ8padUMAfJOuPQNdsg2fRogWpeqzMGqMWJYioN1d2HKkaAFS

For now, I have this installed on the site by adding the following into the composer.json file to pull the modified module:

{
    "require": {
        "integer-net/magento2-session-unblocker": "dev-magento-enterprise-support as v0.1.4"
    },
    "minimum-stability": "stable",
    "repositories": {
        "magento2-session-unblocker": {
            "type": "vcs",
            "url": "https://github.com/davidalger/magento2-session-unblocker"
        }
    }
}

Given these changes introduce a hard dependency on the Commerce/Enterprise only Magento\Banner\Controller\Ajax\Load class in tests/Integration/AbstractSessionTest.php, the integration tests will fail if they are run from an open-source code base. DI rules targeting non-existent classes are merely ignored, so the module will install and run no problem on open-source however (verified this on a vanilla 2.3.3 OS with sample data).

I'm opening this PR both to share the work that I've done, but also ask for ideas on path forward to supporting controllers that do not exist in open-source. Have you guys thought about this yet and what might you suggest to support both open-source and enterprise?

One thought I had is perhaps splitting it to a separate module dependent on this one, keeping them nice and clean but that has the downside of multiple modules to maintain. Alternatively, the tests could be setup so only the open-source tests are run perhaps by using a different phpunit.xml file for enterprise and opensource integration testing where the enterprise one included additional tests, and then splitting AbstractSessionTest::setUpSpies out of the abstract to avoid hard dependencies on irrelevant controller classes.

Thoughts?

@schmengler
Copy link
Contributor

schmengler commented Nov 21, 2019

Thank you very much for your contribution! This brings up an interesting topic, I'd definitely like to support improvements for Magento Commerce, while staying compatible with the open source version.

One thought I had is perhaps splitting it to a separate module dependent on this one, keeping them nice and clean but that has the downside of multiple modules to maintain

I don't see this as a downside at all. In fact I prefer having a single extension, consisting of multiple modules with different dependencies, where you can choose which modules to install.

How would this work?

  • Move the files of the base module IntegerNet\SessionUnblocker to src/SessionUnblocker and create a second module, e.g. IntegerNet\SessionUnblockerEnterpriseBanner in src/SessionUnblockerEnterpriseBanner.
  • The dependency to Magento_Banner should be declared in module.xml of this new module.
  • We can keep the single composer.json in the repository root and may add magento/module-banner as suggested dependency.
  • Make it clear in the installation instructions, that the enterprise module(s) should be explicitly disabled via bin/magento module:disable (or by editing app/etc/config.php)
  • Tests would be separeted in two namespaces as well, and configured as two test suites

@wojtekn
Copy link

wojtekn commented Nov 22, 2019

@schmengler another approach could be about moving it to separate composer module which depends on this one and then there would be two different ways to install it for Open Source and Commerce eg.:

Open Source:

composer require integer-net/magento2-session-unblocker

Commerce:

composer require integer-net/magento2-session-unblocker-commerce

No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants