Skip to content

Conversation

rxu
Copy link
Contributor

@rxu rxu commented Oct 22, 2016

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

PHPBB3-14831.

public function exists($class, $parent, $module)
{
// Clear the Modules Cache first to get actual data
$this->cache->destroy("_modules_$class");
Copy link
Member

@iMattPro iMattPro Oct 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work. Method "destroy" not found in \phpbb\cache\service

Shoud maybe switch to the cache driver interface?

Copy link
Contributor Author

@rxu rxu Oct 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how did it work before in the add() method and why tests didn't fail on that, just curious. These things with migrations are all weird 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VSEphpbb @rxu method destroy exists for each cache driver and works OK because of this.
Probably IDEs cannot handle this case at the moment.

@iMattPro
Copy link
Member

iMattPro commented Oct 22, 2016

I guess don't change the cache then...All the other tools use the service too. My IDE just pointed that out, but it could be a magic method. Seems tests fail after changing it.

@rxu
Copy link
Contributor Author

rxu commented Oct 23, 2016

It looks like this needs more work as there're some more issues with migrations relying on if module.exists conditions.

@rxu
Copy link
Contributor Author

rxu commented Oct 23, 2016

The particular problem is that in case parent_id is not found the get_parent_module_id() throws exception before the exists() method returns anything out.

public function get_parent_module_id($parent_id, $data = '', $throw_exception = true)
{
// Initialize exception object placeholder
$e = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please a better name than just $e

@rxu
Copy link
Contributor Author

rxu commented Oct 23, 2016

I hope this can be !unset WIP now.

$parent = $this->get_parent_module_id($parent, $module);
$parent_sql = 'AND parent_id = ' . (int) $parent;
$parent = $this->get_parent_module_id($parent, $module, false);
if ($parent !== false)
Copy link
Contributor

@lavigor lavigor Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rxu maybe just

if ($parent === false)
{
    return false;
}
$parent_sql = 'AND parent_id = ' . (int) $parent;

to avoid duplicated construction and simplify the code a little bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, agreed.

Nicofuma added a commit to Nicofuma/phpbb that referenced this pull request Oct 24, 2016
[ticket/14831] Fix extension modules installation issues

* rxu/ticket/14831:
  [ticket/14831] Optimize code construction
  [ticket/14831] Add more tests against UCP modules
  [ticket/14831] Add more tests, better name for $e placeholder
  [ticket/14831] Do not throw exception on the module existence checking
  [ticket/14831] Fix module migrator tool
@Nicofuma Nicofuma merged commit 77f1bac into phpbb:3.1.x Oct 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants