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

Allow vendor usage for libraries #18820

Merged
merged 5 commits into from
Mar 17, 2018
Merged

Conversation

phproberto
Copy link
Contributor

@phproberto phproberto commented Nov 23, 2017

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:

// How we are already doing it
JLoader::import('joomla.filesystem.file');

// How a vendor library class could be loaded
JLoader::import('phproberto.my_library.some_class');

Summary of Changes

This modifies the library installer to allow that manifests can use:

<libraryname>phproberto/sample</libraryname>

instead of:

<libraryname>sample</libraryname>

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

  1. Install Sample Vendor Library
  2. Check that description of the library is translated in the postinstallation message.
  3. Check that the library is found in libraries/phproberto/sample
  4. Go to extension manager and check that library is listed correctly.
  5. In your frontend template index.php file add this lines to test library usage:
// Test library loading
JLoader::import('phproberto.sample.library');

// Test library class usage
$class = new \Phproberto\Joomla\Sample\TestClass;

// This echoes a library language string to test it
echo $class->hello();

You should see a message like:
Hello!! This is a sample language string loaded from a vendor library

Test library uninstall

  1. Go to extension manager and search for Phproberto. You should see the library listed.
  2. Uninstall the library.
  3. Ensure that uninstallation works.

Test library discover install

  1. Go to extension manager and install the sample library from the zip provided.
  2. In your database editor search for the library in the #_extensions table.
  3. Delete the library row.
  4. Go to extension manager > Discover and click discover.
  5. Ensure that the extension is shown with its name translated
  6. Select the extension and click Install
  7. Ensure that the installation finished correctly.

Additional suggested tests

  1. Install a third party library to ensure install/uninstall still works

Documentation Changes Required

Update library manifest information to add information to use the vendor folder.

@rdeutz rdeutz self-assigned this Nov 27, 2017
@tonypartridge
Copy link
Contributor

I have tested this item ✅ successfully on 9e4107c

Great bit of house keeping :-).

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.

@dgrammatiko
Copy link
Contributor

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.

@dgrammatiko
Copy link
Contributor

RTC, Thank you @phproberto


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820.

@tonypartridge
Copy link
Contributor

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.

@dgrammatiko
Copy link
Contributor

@phproberto what will happen if sample is already installed and you install random\sample?
I guess it would be nicer to always require the phproberto/sample structure, and that can only happen in J4

@phproberto
Copy link
Contributor Author

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.

@tonypartridge
Copy link
Contributor

tonypartridge commented Nov 27, 2017 via email

@phproberto
Copy link
Contributor Author

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.

@brianteeman
Copy link
Contributor

What happens in this scenario
Component 1 installs library x
Component 2 installs library x

Uninstall component 1 now also uninstalls library x leaving component 2 broken

@phproberto
Copy link
Contributor Author

@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.

@brianteeman
Copy link
Contributor

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)

@tonypartridge
Copy link
Contributor

I have tested this item ✅ successfully on 7c8356f

Tested working as it was, but now also deletes the parent directly and only if it's empty. Tested with the provided sample package and duplicating the sample package to test under a group with more than 1 library.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18820.

@tonypartridge
Copy link
Contributor

@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.

@phproberto
Copy link
Contributor Author

phproberto commented Nov 28, 2017

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 phproberto/sample but I don't uninstall phproberto/another_lib it won't be removed. But if I have only a phproberto/sample and I uninstall it you won't have an empty /libraries/phproberto folder which was what @tonypartridge reported.

I'm not changing anything outside that. Library uninstaller already uninstalls libraries (which looks like the right thing :D).

@brianteeman
Copy link
Contributor

ok - just wanted to be certain and avoid issues down the line

@tonypartridge
Copy link
Contributor

tonypartridge commented Nov 28, 2017 via email

@dgrammatiko
Copy link
Contributor

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.

@dgrammatiko
Copy link
Contributor

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.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 28, 2017
@phproberto
Copy link
Contributor Author

Thank you very much for testing @dgt41, @tonypartridge & @brianteeman !

@mbabker mbabker added this to the Joomla 3.9.0 milestone Dec 2, 2017
@mbabker mbabker changed the base branch from staging to 3.9-dev March 17, 2018 16:00
@mbabker mbabker merged commit 3943099 into joomla:3.9-dev Mar 17, 2018
@joomla-cms-bot joomla-cms-bot added PR-3.9-dev and removed RTC This Pull Request is Ready To Commit labels Mar 17, 2018
@rdeutz rdeutz removed their assignment Mar 18, 2018
@zero-24 zero-24 removed the PR-staging label May 4, 2018
@phproberto phproberto deleted the lib-vendor branch June 6, 2018 06:33
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.

8 participants