JInstallerAdapterComponent bug fixes related to admin menu build #5499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
December 23, 2014 16:44
Changes: * Pass only the extension ID, not the entire extension row. After all, only the ID is used! * Always run rebuild() on the menus table, preventing broken nested sets on this table (now you know why this table breaks all the time)
Changes: * Type hinting for $table * There can be more than one root menu items for a single component if the installation previously failed without rollback (e.g. timeout error) * The extension ID must always be fetched from the #__extensions table, not the old menu items for the same reasons as above * Just do not create the menu if $menuElement not exist (it was a TODO item for as long as I remember developing for Joomla!) * Unnecessary code duplication creating the root menu item * Refactored the code, moving the actual menu item creation in the _createAdminMenuItem method * If a submenu already existed with the same parent item the installation would fail. Now we try removing it first, just like we do for the root menu item.
If there's a timeout or PHP fatal error during a component's installation there are invalid records left behind in #__extensions and #__menus which prevent further installations. We now detect them and remove them to let the installation proceed.
Hi @nikosdion this should make travis happy here: #5499 ``` FILE: ...s/build/joomla/joomla-cms/libraries/cms/installer/adapter/component.php -------------------------------------------------------------------------------- FOUND 8 ERROR(S) AFFECTING 8 LINE(S) -------------------------------------------------------------------------------- 1141 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1142 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1143 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1255 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1256 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1257 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1258 | ERROR | Tabs must be used to indent lines; spaces are not allowed 1878 | ERROR | Expected 2 spaces after the longest type -------------------------------------------------------------------------------- ```
Make travis Happy for #5499
|
<3 |
|
@wilsonge Thanks for the ping. Going to test it now. |
|
@test: Did a variety of testing with components, modules, plugins, and packages from 3rd parties and custom extensions. All installed and uninstalled as expected. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5499. |
|
I guess we can set this RTC... or does it need a PLT discussion first? |
|
Moving to RTC, we have 2 successful tests. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5499. |
wilsonge
added a commit
that referenced
this pull request
Dec 24, 2014
JInstallerAdapterComponent bug fixes related to admin menu build
|
Merged. Thanks so much for this Nic! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As requested by @wilsonge by email
Executive summary
This patch for JInstallerAdapterComponent deals with issues manifesting themselves as "Can't build admin menus".
The following intertwined bug fixes are applied:
#__menustable was left broken (inconsistent lft/rgt) because rebuild() never ran against it.#__extensionsand#__menusentries leading to inability to reinstall the componentBackwards compatibility
The protected _removeAdminMenus method has a different signature. To the best of my knowledge nobody extends JInstallerAdapterComponent directly, therefore the impact is non-existent.
Translation impact
None
Testing instructions
As I explained to @wislonge the problems only manifest themselves on bad hosting and when several things go wrong. As a result there is no way to do positive tests which reproduce the bugs. These are "blind" fixes. That said, I've been tracking down these issues since 2007 and applying workarounds on my own extensions ever since. I am pretty darn sure that my code DOES fix these issues.
As a result the only possible testing you can do is regression testing. Try applying this patch and install as many different components as you can on your site (but don't try my extensions, especially the pre-release versions, some have experimental fixes which will not work properly and override this patch in any case!). The installation should succeed and the components should be perfectly usable.