Track the package/extension relationships #12977

Merged
merged 8 commits into from Dec 7, 2016

Conversation

Projects
None yet
8 participants
@mbabker
Member

mbabker commented Nov 22, 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
@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 24, 2016

Contributor

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

Contributor

alikon commented Nov 24, 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

@mahagr

This comment has been minimized.

Show comment
Hide comment
@mahagr

mahagr Nov 24, 2016

Contributor

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

Contributor

mahagr commented Nov 24, 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.

@mbabker mbabker changed the title from [WIP] Track the package/extension relationships to Track the package/extension relationships Nov 25, 2016

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Nov 25, 2016

Member

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.

Member

mbabker commented Nov 25, 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.

@mahagr

This comment has been minimized.

Show comment
Hide comment
@mahagr

mahagr Nov 25, 2016

Contributor

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

Contributor

mahagr commented Nov 25, 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...

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Nov 25, 2016

Contributor

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.

Contributor

Bakual commented Nov 25, 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.

@@ -138,8 +146,10 @@ protected function copyBaseFiles()
);
}
+ $this->installedIds[] = $tmpInstaller->extension->extension_id;

This comment has been minimized.

@alikon

alikon Nov 26, 2016

Contributor

$tmpInstaller->extension->extension_id; is always null

@alikon

alikon Nov 26, 2016

Contributor

$tmpInstaller->extension->extension_id; is always null

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 26, 2016

Contributor

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 ?

Contributor

alikon commented Nov 26, 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 ?

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Nov 26, 2016

Member

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

Member

mbabker commented Nov 26, 2016

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

@@ -0,0 +1 @@
+ALTER TABLE "#__extensions" ADD COLUMN "package_id" bigint DEFAULT 0 NOT NULL;

This comment has been minimized.

@andrepereiradasilva

andrepereiradasilva Nov 26, 2016

Contributor

minor issue but for consistency across db systems you should add a comment here too.

COMMENT ON COLUMN "#__extensions"."package_id" IS 'Parent package ID for extensions installed as a package.';
@andrepereiradasilva

andrepereiradasilva Nov 26, 2016

Contributor

minor issue but for consistency across db systems you should add a comment here too.

COMMENT ON COLUMN "#__extensions"."package_id" IS 'Parent package ID for extensions installed as a package.';
@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 27, 2016

Contributor

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
Contributor

andrepereiradasilva commented Nov 27, 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
@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Nov 27, 2016

Member

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.

Member

mbabker commented Nov 27, 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.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 27, 2016

Contributor

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

Contributor

andrepereiradasilva commented Nov 27, 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

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Nov 27, 2016

Member

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.

Member

mbabker commented Nov 27, 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.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 27, 2016

Contributor

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

Contributor

andrepereiradasilva commented Nov 27, 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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 27, 2016

Contributor

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'));
Contributor

andrepereiradasilva commented Nov 27, 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'));
@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Nov 27, 2016

Member

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.

Member

mbabker commented Nov 27, 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.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 27, 2016

Contributor

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

Contributor

andrepereiradasilva commented Nov 27, 2016

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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 27, 2016

Contributor

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.

Contributor

andrepereiradasilva commented Nov 27, 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.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 28, 2016

Contributor

works as expected on fresh install
manage

and with pkg's

managepkg

Contributor

alikon commented Nov 28, 2016

works as expected on fresh install
manage

and with pkg's

managepkg

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 28, 2016

Contributor

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.

Contributor

alikon commented Nov 28, 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.

@jeckodevelopment

This comment has been minimized.

Show comment
Hide comment
Member

jeckodevelopment commented Nov 28, 2016

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 28, 2016

@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Nov 28, 2016

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz Dec 6, 2016

Contributor

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

Contributor

rdeutz commented Dec 6, 2016

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

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Dec 6, 2016

Member

Done

Member

mbabker commented Dec 6, 2016

Done

@rdeutz rdeutz merged commit 1f74751 into joomla:staging Dec 7, 2016

2 checks passed

continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Dec 7, 2016

@mbabker mbabker deleted the mbabker:package-relations branch Dec 7, 2016

@mbabker mbabker added the New Feature label Jan 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment