search

userfrosting / UserFrosting

Modern PHP user login and management framework
https://www.userfrosting.com
Other
1.64k stars 366 forks source link

[5.0] Two sprinkles can't have the same name #1243

Closed lcharette closed 8 months ago

lcharette commented 9 months ago

To reproduce

  1. Do a clean Install
  2. Run command php bakery debug:locator
$ php bakery debug:locator

 ------------------ ------------------ ----------------------------------------------------------------------------- 
  Name               Slug               Path                                                                         
 ------------------ ------------------ ----------------------------------------------------------------------------- 
  My Application     my-application     /Users/malou/Desktop/UserFrosting/app/                                       
  Admin Sprinkle     admin-sprinkle     /Users/malou/Desktop/UserFrosting/vendor/userfrosting/sprinkle-admin/app/    
  AdminLTE Theme     adminlte-theme     /Users/malou/Desktop/UserFrosting/vendor/userfrosting/theme-adminlte/app/    
  Account Sprinkle   account-sprinkle   /Users/malou/Desktop/UserFrosting/vendor/userfrosting/sprinkle-account/app/  
  Core Sprinkle      core-sprinkle      /Users/malou/Desktop/UserFrosting/vendor/userfrosting/sprinkle-core/app/     
 ------------------ ------------------ -----------------------------------------------------------------------------

everything works fine

  1. Change Sprinkle name from My Application to Admin Sprinkle
  2. Run command php bakery debug:locator again 
    ------------------ ------------------ ----------------------------------------------------------------------------- 
Name               Slug               Path                                                                         
------------------ ------------------ ----------------------------------------------------------------------------- 
Admin Sprinkle     admin-sprinkle     /Users/malou/Desktop/UserFrosting/app/                                       
AdminLTE Theme     adminlte-theme     /Users/malou/Desktop/UserFrosting/vendor/userfrosting/theme-adminlte/app/    
Account Sprinkle   account-sprinkle   /Users/malou/Desktop/UserFrosting/vendor/userfrosting/sprinkle-account/app/  
Core Sprinkle      core-sprinkle      /Users/malou/Desktop/UserFrosting/vendor/userfrosting/sprinkle-core/app/     
------------------ ------------------ -----------------------------------------------------------------------------

Notice the original "Admin Sprinkle" is gone. All Admin routes returns an error.

Changing Sprinkle name from My Application to Core Sprinkle is even worst, since default configuration files are not found anymore.

The cause

Routes locations are stores by name here : https://github.com/userfrosting/framework/blob/c71ea13d8d91d0572d0255c2d13464c24baf628c/src/UniformResourceLocator/ResourceLocator.php#L239

And each sprinkle register it's location here, using the sprinkle name as the location name : https://github.com/userfrosting/sprinkle-core/blob/6695e47c182fafe93db93f971433634861ba1a11/app/src/ServicesProvider/LocatorService.php#L38

It doesn't help that slugs are derived from sprinkle name: https://github.com/userfrosting/framework/blob/c71ea13d8d91d0572d0255c2d13464c24baf628c/src/UniformResourceLocator/ResourceLocation.php#L55-L58

And finally Twig namespaces are using the route slugs (which is not ideal on it's own) : https://github.com/userfrosting/sprinkle-core/blob/6695e47c182fafe93db93f971433634861ba1a11/app/src/ServicesProvider/TwigService.php#L50

This is obviously an issue, as the sprinkle name should be for display purpose only. Plus, renaming a sprinkle could break Twig namespace feature, which is no good.

The solution

The solution is a bit complex, but an opportunity for further improvement at the same time.

  1. Location should be store by their unique slug. This means:
    1. Slugs need to be explicitly defined instead of computed from the name;
    2. removeLocation and getLocation needs to be changed from name to slug as the argument [BC]
  2. However, this also means sprinkles needs to have their own unique "slug"
    1. Composer {vendor}/{package} format could be used as our sprinkle unique slugs. This could be helpful to get info from composer about each sprinkle (eg. installed version of a sprinkle for debug purposes). Another solution would be the recipe FQN, but this wouldn't work with the Twig namespace thing.
    2. The unique slug will need to be explicitly defined in the sprinkle recipe, unless there's a better way to do this.
    3. Sprinkle manager will need to be able to return each sprinkle unique slug, and make sure the same slug is not defined twice
  3. Twig namespace is currently defined by the Location's slug, which is the sprinkle's name. Changing the slugs create another Breaking Change in [Core-Sprinkle]
  4. Since locator's locations are not exclusive to sprinkles (a sprinkle could register a second location for whatever reason), whatever solution needs to take that into account. So sprinkle main path as a location is still vlid,

The other solution would be to store Locations not based on anything (add them to a simple array instead of associative array). However, this solutions means it's ~not longer possible~ way harder for a location to be removed.

Location path could also be used instead of slug, but I'm not sure it's the right solution. For one, Twig namespace still needs be be tied to the location, not the sprinkle, as a sprinkle can still have a second, manual, location...

Fixing this will require a breaking change in both Framework and Core-sprinkle , so it can't be fixed on the 5.0 branch, hence marking it for 5.1. I'm creating this issue to track progress on this, and as a note if anyone encounter this issue.

lcharette commented 8 months ago

~After looking further into this, the following solution has been implemented for 5.1 release :~

~1. Locator's location must have unique names, otherwise an exception will be thrown (fix a real issue with locator);

  1. While at it, Location slugs have been deprecated, since it was not used, and redundant with the name;
  2. Sprinkles recipe can now (optionally) define a string in their recipe, representing the composer package name (all default sprinkle will have this);
  3. The composer package name will be used to display the sprinkle version in sprinkle:list bakery command;
  4. The composer package name will also be used as the locator name, and thus the Twig namespace. If the composer package name isn't defined (because it's optional), the sprinkle name will be used as fallback, with
  5. To help debug, a new debug:twig bakery function has been added. It displays each namespace, with the path it point to.~

~tl;dr : Two sprinkle can have the same name, as long as they don't have the same package name (which shouldn't be, because of Composer). Plus, Twig namespace will be changed from @adminlte-theme to @userfrosting/theme-adminlte.~

lcharette commented 8 months ago

Scratch that, Twig namespaces doesn't support / in it 🤦‍♂️

lcharette commented 8 months ago

Alright, let do this once again.

After looking further into this, the following solution has been implemented for 5.1 release :

  1. Locator's location must have unique names, otherwise an exception will be thrown (fix a real issue with locator);
  2. While at it, Location slugs have been deprecated, since it was not used, and redundant with the name;
  3. The sprinkle's name will be converted to a slug (like before) and used as the locator location name, and thus the Twig namespace.
  4. To help debug, a new debug:twig bakery function has been added. It displays each namespace, with the path it point to.

tl;dr : Two sprinkle still can't have the same name, but now a proper exception will be thrown instead of silently removing a location. Twig namespace can be found using debug:twig.