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

Auto register ide-helper hook using Package Discovery #215

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

daniel-de-wit
Copy link
Contributor

Would you consider automatically registering the model hook using Package Discovery?

@staudenmeir
Copy link
Owner

Hi @daniel-de-wit,
Thanks, I'll take a look.

This package should not be installed on a production environment, therefore we don't need to check it.
…ider

Ensures the hook is only registered after the ModelsCommand is called.
@daniel-de-wit
Copy link
Contributor Author

@staudenmeir I've updated the PR to make the service provider deferred, just like the one from IDEHelper. This ensures the order of package discovery doesn't effect the registration of the model hook. Now it only registers after the ModelsCommand is requested from the service container, which removed the need for a couple of environment guards.

I would like to hear your take on it

# Conflicts:
#	composer.json
@staudenmeir staudenmeir merged commit 9d9aa83 into staudenmeir:master Dec 4, 2023
13 of 14 checks passed
@staudenmeir
Copy link
Owner

Thanks, excellent PR! I've released a new version.

@staudenmeir
Copy link
Owner

One last question: You kept and even improved the README section about manually enabling the model hook.

Do you still see a use case for that?

@daniel-de-wit
Copy link
Contributor Author

I added that for people that have package discovery disabled. I never encountered a project that disabled the entire package discovery though.

@staudenmeir
Copy link
Owner

Ok, thanks.

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

Successfully merging this pull request may close these issues.

2 participants