Skip to content

Graceful component registrar #4044

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

Conversation

joshdifabio
Copy link
Contributor

Some workflows can result in ComponentRegistrar::register() being called multiple times for the same component. In order to avoid messy, repeated boilerplate in client code, it makes sense for the register() method to be null-op in cases where the component has already been registered with the same path.

Related to #3871

Some workflows can result in register() being called multiple
times for the same component. In order to avoid messy boilerplate
in client code, it makes sense for the register() method to be
null-op in cases where the component has already been registered
with the same path.
If the component has already been registered then $type must be valid
@vkublytskyi
Copy link

If some workflow register same component several time there is an issue in workflow. So the reason why same component (from same path) registered more than once should be fixed.

Initial PR #3871 tries to solve other issue when Magento installation have several copies of same component in different folders (e.g. in app/code and vendor)

@vkublytskyi
Copy link

In any case thank you for your impact on this issue. Let's collaborate and find optimal solution

@joshdifabio
Copy link
Contributor Author

Initial PR #3871 tries to solve other issue when Magento installation have several copies of same component in different folders (e.g. in app/code and vendor)

I see. In that case, as you said, this PR isn't relevant to that issue. I'll stay out of that one as I don't have any experience of such a scenario.

Putting that issue aside, I still see this PR as an improvement; what is the value in throwing an exception in the case that a component is registered twice with the same directory path? Will we start seeing developers wrap their register() calls in a try-catch and checking the existing path in the catch block? Does it not make more sense for register() to be idempotent?

@mazhalai
Copy link
Contributor

mazhalai commented May 6, 2016

@joshdifabio please sync with the develop branch and rerun travis builds.

@joshdifabio
Copy link
Contributor Author

If at any point it's decided to merge this then I'll be happy to do so, but it's a waste of my time otherwise.

@buskamuza
Copy link
Contributor

@joshdifabio , could you, please, provide an example of the workflow that leads to registration of the same component multiple times?

@joshdifabio
Copy link
Contributor Author

@joshdifabio , could you, please, provide an example of the workflow that leads to registration of the same component multiple times?

Not off the top of my head, but this is more about throwing an exception in a situation where doing so provides zero benefit. However, it's a complete waste of time (which is extremely precious to many people, myself included) and completely missing the benefits of OSS to spend so much time discussing such minor changes. This PR is absolutely not worth the time being spent on it, so I'm going to close it.

@joshdifabio joshdifabio closed this May 6, 2016
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