-
-
Notifications
You must be signed in to change notification settings - Fork 548
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
[5.x] Autoload add-on tags, widgets, modifiers etc from folder #9270
[5.x] Autoload add-on tags, widgets, modifiers etc from folder #9270
Conversation
My hero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great - Simple Commerce would have a much smaller ServiceProvider
after this gets merged.
One other thing... GitHub wouldn't let me comment on the relevant lines since they weren't changed. Can we add the same sort of auto-loading magic to update scripts? cms/src/Providers/AddonServiceProvider.php Lines 514 to 521 in 51668c9
|
I've added support for UpdateScripts too |
Commands in src/Console/Commands weren't being picked up due to the incorrect FQCN being constructed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I've tested this in a few of my addons and it seems to be working great.
I noticed there was an issue with commands in src/Console/Commands
not being picked up due to an incorrect $fqcn
. I've just pushed up a fix for it.
The initial reason this wasn't ever done was because of a potential performance hit. It's probably negligible, but it's not nothing. On every request, we're now looking for tags, scopes, actions, fieldtypes, modifiers, widgets, commands twice, and update scripts. That's 9 directory traversals, for each installed addon. Have 3 addons installed, and you're looping through directories an extra 27 times per request. I was thinking maybe we could cache these with an I'll do some testing on load time differences before/after this PR when I have some time. If someone else also wants to, and share their results, that'd be helpful. Maybe it'll turn out to not be a big deal after all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
At first glance it's not really making a dent. Maybe this is a non-issue. I'm using an M1 Mac though which is probably faster than most servers. |
I'm happy to work on the cache command if you feel it's required, no problem either way. |
@jasonvarga just a proof of concept and would need refined, but you could use the existing addon manifest cache to do it... something like 0114c4c |
This reverts commit 0114c4c.
@edalzell wanted this...
This PR autoloads tags, scopes, modifiers, actions, fieldtypes, widgets and commands based on them being in an appropriately named directory within the add-on src directory.
eg src/Tags/MyTag.php will automatically register, so you dont need to add it to the $tags array.
Closes statamic/ideas#88
Closes statamic/ideas#709