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

[8.x] Generalized stub path resolution #37412

Closed

Conversation

Radiergummi
Copy link

@Radiergummi Radiergummi commented May 19, 2021

This PR solves the issue described in #37389: Several make: commands do not use custom stubs in a project but are hard-wired to their default stub files instead.
While creating the PR, I discovered several different ways to resolve the stub path in the existing code, which have all been reduced to a single protected method in the GeneratorCommand, leading to a net negative of lines and simplified code. This new resolveStubPath helper uses a reflection class to locate the directory of the current class, which I deemed acceptable for a command intended to be used during development. This makes it possible to let all generator commands (even third-party ones) use the same, uniform way to locate stubs, while preserving backwards-compatibility.

Please note that this PR does not change which stubs are published and which aren't, as I think that's out of scope and not my decision to make. It merely gives users the option to override any stubs to fit their own requirements, if that is necessary.

While there are no tests for generator commands as far as I could see, I did run the test suite without issues and monkey-patched the changes in a standard Laravel project to verify path resolution is working as intended.

@taylorotwell
Copy link
Member

It's not really an oversight - I simply created stubs for the things that are most commonly customized. This PR is also not quite complete as the stubs themselves would need to be updated as well to use the new {{class}} placeholders, etc.

@Radiergummi
Copy link
Author

@taylorotwell I don't see harm in letting users customize less commonly customized things without any impact for everyone else. Enforcing style guides in a business setting is annoying if code reviewers waste time to point out missing things from automatically generated stuff, if the code generator could easily include those things.

Any chance on the PR being merged if the stubs were updated to include proper placeholders?

@selcukcukur
Copy link
Contributor

@Radiergummi With this flexibility, new needs will arise and I think it will be accepted when you send a pr that meets them.

  1. It should be possible to add custom tags to read from stub files.
  2. When these tags are present in both the original stub and the custom created stub file, it should be able to correctly decide which one to prefer.
  3. Also, apart from the special tags, the namespace in the scripts has a fixed namespace as you know. It may be a good idea to make it customizable too, with this pr.

@Radiergummi
Copy link
Author

@selcukcukur I'm not quite sure what you mean by custom tags? This PR was primarily intended to resolve all stubs using the same mechanism that looks for them in the custom stubs directory or falls back to the stub included with the command, as per Laravel convention. As a side effect, that would simplify the code around stub path resolution.
This would impact nothing existing, but make it possible to swap out stubs for all generator commands, not just an arbitrarily chosen subset.

If there's sufficient interest in supporting this at all, I'd gladly expand the existing, non-customizable stubs to support the same set of placeholders the customizable stubs do and add them to the list of published stubs in stub:publish. I previously thought that would be a separate change, more fit for a second PR, with more discussion.

I won't waste time on the implementation if it's not going to be merged anyway, though.

@selcukcukur
Copy link
Contributor

@Radiergummi By labels I mean placeholders. I can gladly help implement them I have time now, but I have doubts that it will be implemented so it might be a good idea to get a confirmation.

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.

3 participants