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

Add a way for sprinkles to add custom locator path #853

Closed
lcharette opened this issue Feb 16, 2018 · 4 comments
Closed

Add a way for sprinkles to add custom locator path #853

lcharette opened this issue Feb 16, 2018 · 4 comments
Assignees
Labels
architecture Related to the framework architecture core feature request Feature request needs discussion A decision needs to be taken by the dev team
Milestone

Comments

@lcharette
Copy link
Member

Sprinkles can't actually add locator path, even when extending the service. Both locator and streamBuilder are frozen when trying to extend them and even if they weren't, the ressources path still need to be added to the SprinkleManager.

Moreover, listing the locator path 3 times (One, two, three) looks like bad code to me. Using a config value or a dedicated service to return the list of path could probably fix this bad code, and the issue altogether. Problem is, a path requires 3 infos : The slug, the read/write status (currently defined in System/ServicesProvider) and the actual path (Currently defined in the SprinkleManager), so a simple key=> value array in config can't do it.

Also, I'm not sure why we don't use the locator to find the migrations classes, even if they are in src/, instead of the filesystem ?

@lcharette lcharette added core feature request Feature request architecture Related to the framework architecture needs discussion A decision needs to be taken by the dev team labels Feb 16, 2018
@lcharette lcharette added this to the 4.3.0 milestone Feb 16, 2018
@lcharette
Copy link
Member Author

@alexweissman What is the use of the extra:// location? Was it supposed to be a "and everything else" place? Could a temporary fix for now...

@alexweissman
Copy link
Member

Was it supposed to be a "and everything else" place?

I believe so. I made it when I needed a place to put dictionary files for the username generation tool (https://github.com/userfrosting/UserFrosting/releases/tag/4.0.8-Alpha).

@lcharette
Copy link
Member Author

I'm starting to wonder if the locator service doesn't need a complete rewrite? On top of this issue, there's the recurrent need to list files across sprinkles (Migrations, Seeds, etc) which has to be done again and again in different locator class. While the current locator can find a file across sprinkle and list all place where stuff can be found, it could also directly list all files in a given location, in a standard, built in way...

@lcharette lcharette modified the milestones: 4.3.0, 4.2.0 Apr 5, 2018
@lcharette
Copy link
Member Author

lcharette commented Apr 24, 2018

I did some more digging on this. Another issue is the locator service can't actually be extended to add a new path/stream. This is because the service gets frozen since it's used before being extended since the sprinkleManager service is the very first service loaded and it depends on the locator service.

Sprinkles could still be able to add new stream/path (and some definition should probably be moved out of the "system" service provider) but not by extending the locator service. It could be done (manually) in the sprinkle init class, unless someone has a better idea ?

@lcharette lcharette self-assigned this Sep 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Related to the framework architecture core feature request Feature request needs discussion A decision needs to be taken by the dev team
Projects
None yet
Development

No branches or pull requests

2 participants