-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Allow vendor usage for libraries #18820
Conversation
I have tested this item ✅ successfully on 9e4107c Tested using test instructions provided and also with our own library which we already partially use this method in terms of folder nesting/naming. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
I have tested this item ✅ successfully on 9e4107c This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
RTC, Thank you @phproberto This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
I've just noticed something which is semi related to this PR @phproberto if you uninstall the library the parent folder is left behind even if it contains nothing. I don't really see it as an issue since it contains nothing, but worth noting nontheless. |
@phproberto what will happen if |
Thanks for testing!!! @tonypartridge I thought about that when testing it but forgot to invest some time on it after having everything working. Ideally after uninstalling the library the system should be exactly as before installing it. I will send a patch. @dgt41 that cannot be an issue because you will never have such conflict. About forcing this behaviour for v4 I wouldn't recommend it because that will force all the devs to rewrite all their libraries for an aesthetic change. AFAIK v4 is a soft transition so I'd recommend that the old library system works in deprecated mode warning users that v5 will remove support for libraries without vendor. |
Great Roberto and completely agree re J4.
…On 27 Nov 2017, 21:34 +0000, Roberto Segura ***@***.***>, wrote:
Thanks for testing!!!
@tonypartridge I thought about that when testing it but forgot to invest some time on it after having everything working. Ideally after uninstalling the library the system should be exactly as before installing it. I will send a patch.
@dgt41 that cannot be an issue because you will never have such conflict. lib_sample and lib_phproberto_sample are identified as a totally different extensions. One would have element sample and the other one phproberto/sample. They would be also loaded with a different string because the folder structure.
About forcing this behaviour for v4 I wouldn't recommend it because that will force all the devs to rewrite all their libraries for an aesthetic change. AFAIK v4 is a soft transition so I'd recommend that the old library system works in deprecated mode warning users that v5 will remove support for libraries without vendor.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
9e4107c
to
7c8356f
Compare
I've updated the PR. Now it tries to delete vendor folders on uninstall (manifest & library folders) so we don't have empty vendor folders. |
What happens in this scenario Uninstall component 1 now also uninstalls library x leaving component 2 broken |
@brianteeman I don't think that's an issue related to this feature. What happens now (without vendors) in the same scenario? Same issue. Devs already have to take care of those things. |
iirc currently the component uninstall doesnt remove the library (or they chose not to) if i read your comments correctly the library is automatically uninstalled (maybe i need more coffee but I wanted to raise this to ensure that its not a problem) |
I have tested this item ✅ successfully on 7c8356f This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
@brianteeman if that developer is doing that then they should have a check against their component that none of it's other components are installed. That same issue applies right now even without this patch. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
This isn't really changing current behaviour @brianteeman. Probably what confused you is the folder removal. That's removing the vendor folder when there aren't libraries on it anymore. So if I uninstall I'm not changing anything outside that. Library uninstaller already uninstalls libraries (which looks like the right thing :D). |
ok - just wanted to be certain and avoid issues down the line |
Thanks @brianteeman! Always best to be check for sure :D
…On 28 Nov 2017, 09:52 +0000, Brian Teeman ***@***.***>, wrote:
ok - just wanted to be certain and avoid issues down the line
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I have tested this item ✅ successfully on 7c8356f This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
RTC, down to the release leader for the final decision This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820. |
Thank you very much for testing @dgt41, @tonypartridge & @brianteeman ! |
Currently libraries are intended to be stored in
/libraries/{NAME}
which can lead to conflicts between library names and can make the libraries folder a total mess if you have a lot of libraries installed.Wouldn't be awesome that developers could use it's own vendor library folder? Something like what we already use for libraries/joomla that contains a set of "sub-libraries".
The main idea is that devs could have a folder with an structure like:
libraries
phproberto
my_library
another_library
Seeing that loader already supports loading such libraries I thought that it shouldn't be complicated so I decided to play with it a couple of hours. Sample loading calls:
Summary of Changes
This modifies the library installer to allow that manifests can use:
instead of:
I was surprised when I saw that the installer already supports my idea partially. The library installer is already exploding the
libraryname
to copy files into a subfolder:https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Installer/Adapter/LibraryAdapter.php#L230
So this only modifies the installer to update manifest destination, discover install, load language string and extension appearance in extension manager.
This pull request also fixes a side bug I found when testing that everything works correctly: when a library stores language files inside its library folder (example:
libraries/phproberto/sample/language
) those language files weren't automatically loaded in extension manager. They are loaded for other extensions like components, modules, plugins and templates.Testing Instructions
I'm providing an sample vendor library for testing purposes:
sample-vendor-library.zip
Test library installation/usage
libraries/phproberto/sample
You should see a message like:
Hello!! This is a sample language string loaded from a vendor library
Test library uninstall
Phproberto
. You should see the library listed.Test library discover install
#_extensions
table.Install
Additional suggested tests
Documentation Changes Required
Update library manifest information to add information to use the vendor folder.