[#5514] - Continued JInstallerAdapter implementation - Standard Install Routine
- Closed
- 30 Dec 2014
- Medium
- Build: staging
- # 5514
- Diff
- mbabker:MoreInstallAdapter
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Continuing on with the internal refactoring of the extension install adapters, this PR completely standardizes the install routine for all extension adapters minus the language adapter (I'll need to work with that adapter some, it isn't forgotten about).
Summary
In JInstallerAdapter, a standardized install() method is introduced with a standardized path for all extensions. No adapter overrides this method, meaning all adapters are now expected to have the same features for the install routine. The routine is broken into several smaller methods which are standardized across the adapters and reused as often as able; more optimization is surely possible as there's still quite a bit of duplicated code. Additionally, these updates now give library and template extensions the ability to utilize install scripts during the install routine (support to be expanded as the adapters are refactored). In short, if one extension type supports it, everything is supporting it right now.
As an added bonus, some internal refactoring of JInstaller::loadAdapters() has been performed. The method now proxies to a newly preferred API which is better suited for autoloading of adapters (breaking some of the dependency on the JAdapter class) and introduces support for custom extension types. Calling JInstaller::getAdapters and passing an array of adapter names as the second parameter will allow you to create new adapters. For extensions which install addon extensions via their install script, something like this is now possible:
// Instantiate the installer object
$installer = new JInstaller;
// Preload the extension adapters and inject our custom adapter
$installer->getAdapters(array(), array('custom'));
// Install our extension
$result = $installer->install(__DIR__ . '/extension');The API will expect a class named JInstallerAdapterCustom using the above example.
Testing Instructions
Note that EVERY extension type will need to be tested. Templates, components, libraries; you name it. Grab your favorite extensions, install them, and make sure everything checks out with the install routine. Note that this is only focusing on install via the Extension Manager -> Install view. Discover installs are not yet covered, but should be tested to make sure everything is working as expected. Try some updates too if able as a couple of adapters use the install routine for their updates. And of course, everything should cleanly uninstall when you're done testing.
Language Strings
In an effort to clean up some of the duplication, I've added some strings which had already existed in an extension specific context. My aim is that with 4.0, most of the adapter specific strings could be removed in favor of the generic strings and lighten some of the load on translators. We'll cross that bridge later.
jissues-bot
- | Labels |
Added:
?
|
||
One small correction in the new en-GB string please - commented inline
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5514.
| Category | ⇒ | Installation |
Please also update the frontend en-GB.lib_joomla.ini
Done
Warning!: this breaks installing multiple languages at the same time.
We have
[26-Dec-2014 09:09:30 UTC] PHP Warning: array_merge(): Argument #1 is not an array in ROOT/libraries/cms/installer/helper.php on line 152
and
[26-Dec-2014 09:09:30 UTC] PHP Warning: Invalid argument supplied for foreach() in ROOT/libraries/cms/installer/helper.php on line 205
Fixed. The updated code is designed to not used cached singleton instances and com_installer's languages model was doing just that for looping over each of the packages (FWIW, the internals of the package adapter have always created new JInstaller instance to loop through its listed extensions). So I've updated that model to use a new JInstaller instance on each iteration. Also while troubleshooting, I found that JInstallerHelper::unpack() isn't handling a false return on its call to JArchive::extract(), which if it were, the installer would have aborted before reaching the lines with the PHP warnings. I've added a check on that too.
This solves the issue when installing multiple lang packs from the Extension Manager, but it is still broken when installing multiple lang packs at Joomla install time.
Here, this looks like working:
diff --git a/installation/model/languages.php b/installation/model/languages.php
index a2b790a..9ac4293 100644
--- a/installation/model/languages.php
+++ b/installation/model/languages.php
@@ -133,9 +133,10 @@
/* @var InstallationApplicationWeb $app */
$app = JFactory::getApplication();
- $installer = JInstaller::getInstance();
// Loop through every selected language.
foreach ($lids as $id)
{
+ $installer = new JInstaller;
+
// Loads the update database object that represents the language.
$language = JTable::getInstance('update');
Needs tests.
But, some issues apparently in manifests.
Installed 3 languages: Catalan, Chinese Traditional and Chinese Simplified at Joomla install time.
I get 3 empty folders + the xmls:
same when I install French from Extension Manager.
Before the patch, the empty folders are not created.
Added the fix for the install app and cleaned up the empty folder creation.
FWIW, we register that path as the extension root to allow packages to use install scripts and have a place to copy them so they can be used for the same cases as other extension types. With the standardized processing, the extension root path is always getting created. For now, I've overridden the method for the package adapter and moved the logic into the appropriate place in the package adapter so it's once again only created when an install script is present. Just note that if we expand the adapter to support copying more files into a common place, we'll need to basically revert 22ec4a8 and everything will be fine.
@infograf768 can you please confirm there are no more outstanding language install issues please?
All OK now here. Merging. Thanks!
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-12-30 06:39:45 |
I was about to send in a PR for back-porting FOFDatabaseInstaller into JInstaller. However, if your PR goes in I have to wait for this to happen and rewrite my own PR from scratch (which is a good thing, see below). Therefore I'm commenting here, hoping that GitHub will give me a heads up when it gets merged. I know, you need two testers. I'll try to test it once I get over the Christmas hangover.
Michael, THANK YOU for this PR. The biggest brainfsck in adding features to installer adapters was that every single one of them seemed to reinvent the same wheel three to five times over, times eight adapters. I would love to see your PR to be merged and me rewrite my code from scratch. You'll be making my life so damn easier that I can't thank you enough. I guess that's the, um, third beer I have to buy you in JaB15 for making my life easier? :D