? ? ? Success

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
22 Nov 2016

Summary of Changes

Presently there is no tracking in the database of a relationship between a package extension and the extensions it installs, really the only way to know this information is to process the package extension's XML manifest. This is a step at improving this so we can do other things throughout the API.

Testing Instructions

With the patch applied, installing a package extension (language packages are great for testing here) should record the package extension's ID to the #__extensions table's new package_id column. A query failure setting this info is gracefully handled and essentially results in the current status.

This query will be run during the finaliseInstall step of the install process. This step is run by the install and update paths (discover_install as well but packages don't support this so it's a mute point), so as extension packages are updated after this change is deployed, packages will start to correctly associate themselves with child extensions.

Documentation Changes Required

Note the newly added database column.

TODO

  • Add install schema for the rest of the drivers
  • Add the update schema for all drivers
avatar mbabker mbabker - open - 22 Nov 2016
avatar mbabker mbabker - change - 22 Nov 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Nov 2016
Category Administration Language & Strings SQL Installation Libraries
avatar alikon
alikon - comment - 24 Nov 2016

A relationship between a package extension and the extensions it installs query-able via SQL is a good thing,

a little bit beyond

what about to report this information (for example in Extensions: Manage) ?
extensions manage j370 administration

avatar mahagr
mahagr - comment - 24 Nov 2016

👍 from me on all changes that bind packages together. Right now I'm basically locking my own extensions from the package so they cannot be individually removed (without removing the whole package). Its a hack and I don't like it, so I'd want to get rid of it if possible.

Another related feature for this would be to be able to set dependencies between packages. That would allow to have a better system for #12961 as well, where extensions could only be locked if they are being used by another extension.

avatar mbabker mbabker - change - 25 Nov 2016
The description was changed
Labels Removed: ? ?
avatar mbabker mbabker - edited - 25 Nov 2016
avatar mbabker mbabker - change - 25 Nov 2016
The description was changed
avatar mbabker mbabker - edited - 25 Nov 2016
avatar joomla-cms-bot joomla-cms-bot - change - 25 Nov 2016
Category Administration Language & Strings SQL Installation Libraries SQL Administration com_admin Postgresql MS SQL com_installer Language & Strings Templates (admin) Installation Libraries
avatar mbabker mbabker - change - 25 Nov 2016
The description was changed
avatar mbabker mbabker - edited - 25 Nov 2016
avatar mbabker mbabker - change - 25 Nov 2016
Title
[WIP] Track the package/extension relationships
Track the package/extension relationships
avatar mbabker mbabker - edited - 25 Nov 2016
avatar mbabker mbabker - change - 25 Nov 2016
Title
[WIP] Track the package/extension relationships
Track the package/extension relationships
avatar mbabker
mbabker - comment - 25 Nov 2016

Added all the required schema. For what this PR is it is now feature complete.

what about to report this information (for example in Extensions: Manage) ?

Done.

Right now I'm basically locking my own extensions from the package so they cannot be individually removed (without removing the whole package). Its a hack and I don't like it, so I'd want to get rid of it if possible.

As I hinted at in #12976 a later phase would introduce API to be able to manage this. I haven't thought this through completely yet, but shooting from the hip, it would basically be if you choose to uninstall an extension that has an associated package, it would load that package's data to determine if the package allows individual uninstalls (probably a tag in the manifest schema or an attribute on the base tag similar to method="upgrade", to be determined and programmatically defaulting to existing behavior) and if the package disallows it then the process is aborted. This way individual extensions don't have to make these types of checks on their own (as their minimums come up to where Joomla has this API) and we can remove ugly hacks like what's described in that issue which blocks removing language extensions that are installed without a corresponding package.

Another related feature for this would be to be able to set dependencies between packages. That would allow to have a better system for #12961 as well, where extensions could only be locked if they are being used by another extension.

Inter-extension dependency declarations/tracking isn't in any way related to this. Nor should it be.

avatar mahagr
mahagr - comment - 25 Nov 2016

Nice work @mbabker :) I cannot wait removing my ugly hack on this.

Another question that comes into my mind is: what if package provided optional extension that can be removed and the user chooses to remove it. After a while there's a new version of the package -- so user installs it. Right now the removed extension would not stay uninstalled.

Though its a bit more complicated than this as new extensions could also be added to the package later on. And what would happen if user changes his mind and wants to get it back...

avatar Bakual
Bakual - comment - 25 Nov 2016

A package should always install all parts of the package imho. If the user uninstalls a part, then it should be reinstalled. That's what I would expect.

avatar alikon
alikon - comment - 26 Nov 2016

i've made some test with different pkg's like pkg_weblinks ect...
but the new field package_id is never stored

some bug on installer/adapter ?

avatar mbabker
mbabker - comment - 26 Nov 2016

I'd call that a bug in JTable then if the PK field isn't getting populated on insert.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

same results as @alikon installed several pkgs and i never get the package_id populated.

Also some other things i noticed:

  • Shouldn't the update sql update all package id to 0 so all sites have the default 0 value?
  • Shouldn't the existing pkg_en-GB and en-GB languages have that relation on install and on update?
  • The package_id ordering column doesn't show the arrow on order
  • The package_id column should, IMHO, be width 1% and also with nowrap class
avatar mbabker
mbabker - comment - 27 Nov 2016

i never get the package_id populated

Then there is a bug with the use of JTable populating the PK fields after an insert operation. Unrelated to this pull request. I will not change to use $db->insertid() as we have no way of guaranteeing that the last identifier is indeed the one for the installed extension. Other suggestions are welcome otherwise this bug alone causes this pull request to be unacceptable.

Shouldn't the update sql update all package id to 0 so all sites have the default 0 value?

That's exactly what the NOT NULL DEFAULT 0 segment of the SQL statement does. If we have to execute a manual UPDATE statement after altering the table then the use of this construct cannot be relied on in general and the obvious bug should be reported to the upstream database engines.

Shouldn't the existing pkg_en-GB and en-GB languages have that relation on install and on update?

We cannot guarantee the 802 ID is the correct relation.

The package_id column should, IMHO, be width 1% and also with nowrap class

This is contradictory. The column's "native" width at 1% is 54px, adding the nowrap class makes it 72px. In other words there is no way for the column with its title to have a 1% width and nowrap as the class declaration extends it out to 1.33%. Without any style declarations, the column assumes a "natural" width of 80 pixels and with the arrow it extends out to 104 pixels.

The package_id ordering column doesn't show the arrow on order

Fixed.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

Then there is a bug with the use of JTable populating the PK fields after an insert operation. Unrelated to this pull request. I will not change to use $db->insertid() as we have no way of guaranteeing that the last identifier is indeed the one for the installed extension. Other suggestions are welcome otherwise this bug alone causes this pull request to be unacceptable.

yes i understand is not related to this PR, but the fact is that needs to somehow be fixed, because it doesn't seem to work as it is

That's exactly what the NOT NULL DEFAULT 0 segment of the SQL statement does. If we have to execute a manual UPDATE statement after altering the table then the use of this construct cannot be relied on in general and the obvious bug should be reported to the upstream database engines.

right, sleeping sorry

We cannot guarantee the 802 ID is the correct relation.

you can, on install sql, but not on update, but you can still do a query on update to check for the pkg-en-GB id and use it.

This is contradictory. The column's "native" width at 1% is 54px, adding the nowrap class makes it 72px.

1% makes the column smallest as possible according to the viewport width.
nowrap makes to column never wrap.
https://github.com/joomla/joomla-cms/blob/staging/administrator/templates/isis/css/template.css#L5591

avatar mbabker
mbabker - comment - 27 Nov 2016

yes i understand is not related to this PR, but the fact is that needs to somehow be fixed, because it doesn't seem to work as it is

Turns out it's not a bug in JTable. The JTableExtension instance used by the JInstaller instance is not the same as the one in the JInstallerAdapter object. So, to get the installed extension ID, this now has to hook into the onExtensionAfterInstall event which broadcasts the JInstaller instance and the result of JInstallerAdapter::install(). Funny enough, that's documented to only return boolean, but it actually returns boolean false on a fail condition and the extension ID on success (yay documentation bugs).

As for the column width, honestly I'd have to say we're doing it majorly wrong if we're declaring in the HTML that it should be the smallest width then in the CSS declarations using a class set that is widening that column beyond what the HTML has declared. The HTML width declaration should be consistent with what we are doing with design otherwise why bother with the width attributes in the first place? So if I add this stuff, I will do it only using values that are consistent with the end result.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

As for the column width, honestly I'd have to say we're doing it majorly wrong if we're declaring in the HTML that it should be the smallest width then in the CSS declarations using a class set that is widening that column beyond what the HTML has declared. The HTML width declaration should be consistent with what we are doing with design otherwise why bother with the width attributes in the first place? So if I add this stuff, I will do it only using values that are consistent with the end result.
Show all checks

that is not big issue. not worth pending this PR because of that IMHO

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

reggarding the update sql query for pkg_en-GB
something like this would work, i think

UPDATE `#__extensions` AS `e1`
INNER JOIN (SELECT `extension_id` FROM `#__extensions` WHERE `type` = 'package' AND `element` = 'pkg_en-GB') AS `e2`
SET `e1`.`package_id` = `e2`.`extension_id`
WHERE ((`e1`.`type`= 'language' AND `e1`.`element` = 'en-GB') OR (`e1`.`type`= 'package' AND `e1`.`element` = 'pkg_en-GB'));
avatar mbabker
mbabker - comment - 27 Nov 2016

Update queries done (I'll be so glad when we stop including data changes in the schema changeset files). If someone wants to make sure they're actually right for all engines that'd be helpful.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

misses the pkg_en-GB relation in joomla.sql install files

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

I have tested this item successfully on 0248944

run updated sql manually installed language packages and weblinks and package id is there.


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 27 Nov 2016 - Tested successfully
avatar alikon
alikon - comment - 28 Nov 2016

works as expected on fresh install
manage

and with pkg's

managepkg

avatar alikon
alikon - comment - 28 Nov 2016

I have tested this item successfully on 0248944

on postgresql


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

avatar alikon alikon - test_item - 28 Nov 2016 - Tested successfully
avatar jeckodevelopment jeckodevelopment - change - 28 Nov 2016
Status Pending Ready to Commit
Labels
avatar jeckodevelopment
jeckodevelopment - comment - 28 Nov 2016

RTC


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

avatar jeckodevelopment jeckodevelopment - change - 28 Nov 2016
Milestone Added:
avatar rdeutz
rdeutz - comment - 6 Dec 2016

@mbabker could you have a look at the conflicts, thanks

avatar mbabker
mbabker - comment - 6 Dec 2016

Done

avatar rdeutz rdeutz - change - 7 Dec 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-12-07 08:29:55
Closed_By rdeutz
avatar rdeutz rdeutz - close - 7 Dec 2016
avatar rdeutz rdeutz - merge - 7 Dec 2016
avatar rdeutz rdeutz - reference | 1f74751 - 7 Dec 16
avatar rdeutz rdeutz - merge - 7 Dec 2016
avatar rdeutz rdeutz - close - 7 Dec 2016
avatar mbabker mbabker - head_ref_deleted - 7 Dec 2016

Add a Comment

Login with GitHub to post a comment