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

[ticket/15339] Use manual method when adding modules #4971

Merged
merged 4 commits into from Dec 28, 2017

Conversation

@Elsensee
Copy link
Member

commented Sep 22, 2017

PR for master: #5017

This will fix cases where the module is deleted in later
versions and the migrator thus cannot retrieve the info.

Checklist:

  • Correct branch: master for new features; 3.2.x, 3.1.x for fixes
  • Tests pass
  • Code follows coding guidelines: master / 3.2.x, 3.1.x
  • Commit follows commit message format

Tracker ticket:
https://tracker.phpbb.com/browse/PHPBB3-15339

[ticket/15339] Use manual method when adding modules
This will fix cases where the module is deleted in later
versions and the migrator thus cannot retrieve the info.

PHPBB3-15339

@Elsensee Elsensee added the 3.2 (Rhea) label Sep 22, 2017

@Elsensee Elsensee added this to the 3.2.2 milestone Sep 22, 2017

@Elsensee Elsensee referenced this pull request Oct 28, 2017
3 of 4 tasks complete

@Elsensee Elsensee added the WIP 🚧 label Oct 28, 2017

@marc1706 marc1706 modified the milestones: 3.2.2, 3.2.3 Oct 31, 2017

Elsensee added 3 commits Nov 17, 2017
[ticket/15339] Allow a module to have multiple parents
Also restore old behaviour from Olympus regarding re-sorting modules

PHPBB3-15339
[ticket/15339] Fix tests
PHPBB3-15339
@Elsensee

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2017

This basically reverts https://tracker.phpbb.com/browse/PHPBB3-14703 to get back to the old intended behaviour from 3.0

@marc1706 marc1706 removed the WIP 🚧 label Dec 28, 2017

@marc1706 marc1706 modified the milestones: 3.2.3, 3.2.2 Dec 28, 2017

marc1706 added a commit to marc1706/phpbb that referenced this pull request Dec 28, 2017
Merge pull request phpbb#4971 from Elsensee/ticket/15339
[ticket/15339] Use manual method when adding modules

@marc1706 marc1706 merged commit 2b16b7c into phpbb:3.2.x Dec 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@VSEphpbb

This comment has been minimized.

Copy link
Member

commented Mar 18, 2018

@marc1706 @Elsensee
The changes made here seem to have broken the module exists function. Before if $parent was false/0, then the module would still be checked to see if it exists. Now calls to module.exists with a false/0 parent will always return false.

The result of these changes is that a lot of migrations that use module.exists and module.remove will now fail if they look like:

array('module.exists', array('acp', false, 'ACP_MODULE_NAME')),

Also the fact it just return the negative of the $lazy boolean at the end doesn't seem to make any sense either.

@kasimi

This comment has been minimized.

Copy link
Member

commented Mar 18, 2018

@VSEphpbb

This comment has been minimized.

Copy link
Member

commented Mar 18, 2018

Yep that's definitely an occurence of an extension migration failing now over these changes. It uses:

array('if', array(
    array('module.exists', array('acp', false, 'ACP_SEND_STATISTICS')),
    array('module.remove', array('acp', false, 'ACP_SEND_STATISTICS')),
)),
@VSEphpbb

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

LOL, reading the docblock you can see it won't work:

* @param int|string|bool $parent The parent module_id|module_langname (0 for no parent).
*	Use false to ignore the parent check and check class wide.
* @return bool true if module exists in *all* given parents, false if not

If the return is going to be false unless a module exists in all parents, then what how can we tell if a module exists when we use false for the parent and check class wide?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.