Skip to content

Add a ModuleManagerInterface for determining if a module is enabled #12677

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

Closed
wants to merge 3 commits into from
Closed

Conversation

navarr
Copy link
Member

@navarr navarr commented Dec 13, 2017

This is a useful addition for feature detection and enablement for third party modules (such as "activate this functionality if such and such GiftCard extension exists"

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)

@osrecio osrecio assigned osrecio and unassigned osrecio Dec 13, 2017
@orlangur orlangur self-assigned this Dec 13, 2017
@orlangur
Copy link
Contributor

public function isEnabled($moduleName)
    {
        return $this->moduleList->has($moduleName);
    }

This is the only method of mentioned class. What is the purpose of proposed interface?

@navarr
Copy link
Member Author

navarr commented Dec 13, 2017

Purpose is to add an easy ability to determine if a module is enabled to the API. Currently neither the ModuleManager nor the ModuleListInterface are @api annotated

@orlangur

@orlangur
Copy link
Contributor

Interface is only useful when implementation may be potentially switched. Why not mark class method with @api instead?

@navarr
Copy link
Member Author

navarr commented Dec 13, 2017

I can do that. I defaulted to an interface b/c I figured it'd be what'd be desired - after all, just b/c I can't think of an alternative implementation doesn't mean one can't exist!

Should I PR the addition on the model itself? Doing so would add a couple non-api deprecated methods to the API though

@orlangur
Copy link
Contributor

I was thinking about marking only one method as @api.

Let me check in Slack whether there was some reasoning behind not marking this class as @api and whether we should mark class or method.

@orlangur
Copy link
Contributor

Raised a question in Slack

Is there any reason of not marking https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/Module/Manager.php as @api?

@navarr
Copy link
Member Author

navarr commented Apr 19, 2018

@orlangur I'm guessing you never got any feedback in Slack?

@orlangur
Copy link
Contributor

@navarr yeah, but now we have a better process.

@orlangur orlangur removed their assignment Oct 17, 2018
@orlangur
Copy link
Contributor

@antonkril @buskamuza please take a look on this interface suggestion. Of course it should be done against 2.3-develop but first it would be nice to understand whether such feature detection is an acceptable approach.

@antonkril
Copy link
Contributor

Explicit isModuleEnabled checks are usually a sign of bad modularity.

But yes, such interface with @api annotation makes sense.

Copy link
Contributor

@antonkril antonkril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@navarr navarr changed the base branch from 2.2-develop to 2.3-develop October 22, 2018 16:04
@navarr
Copy link
Member Author

navarr commented Oct 22, 2018

I'm closing this PR to make a new one against 2.3

@navarr navarr closed this Oct 22, 2018
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.

5 participants