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/14938] Inconsistency in ext_mgr all_available vs is_available #4592

Merged
merged 5 commits into from Apr 16, 2017

Conversation

@javiexin
Copy link
Contributor

commented Dec 26, 2016

Made is_available much more strict, in line with the checks in all_available

PHPBB3-14938

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

[ticket/14938] Inconsistency in ext_mgr all_available vs is_available
Made is_available much more strict, in line with the checks in all_available

PHPBB3-14938
@Nicofuma

This comment has been minimized.

Copy link
Member

commented Dec 29, 2016

You could refactor these functions by adding an is_valid_extension($ext_name) or is_valid($ext_name) function. This way the code won't be duplicated

@Nicofuma Nicofuma added this to the 3.1.11 milestone Dec 29, 2016

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2016

In fact, the new is_available could be called from all available, saving the duplicate code as well...
Let me know what to do, and I will push another commit with the changes you recommend.

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

Do what you want, calling is_available() is fine too.

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

Refactored using is_available to implement all_available, saving duplicate code.

$available[$ext_name] = $this->phpbb_root_path . 'ext/' . $ext_name . '/';
}
}
}

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Dec 30, 2016

Member

you have an indentation issue here

This comment has been minimized.

Copy link
@javiexin

javiexin Dec 30, 2016

Author Contributor

Fixed, sorry. Tried amending last commit, let's see if it works.

[ticket/14938] Inconsistency in ext_mgr all_available vs is_available
Made is_available much more strict, in line with the checks in all_available
Refactor all_available to use is_available, saving duplicate code.

PHPBB3-14938

@javiexin javiexin force-pushed the javiexin:ticket/14938 branch from 3559cea to 9658eca Dec 30, 2016

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

Again, timeout errors on quicklinks tests, unrelated to this PR.

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

that's expected, some tests are failing due to phpbb.com maintenance

@javiexin javiexin referenced this pull request Jan 14, 2017
3 of 4 tasks complete
@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2017

When ýou look at it, take into consideration that for 3.2, the code for is_available could be greatly simplified by replacing all current code by a call to metadata_manager->get_metadata, with more complete results, and more consistent with the usage of the call.
This cannot be done in 3.1 as the metadata_manager instance requires the template object to be created (constructor param) while this requirement is removed in 3.2 (rightly in my view, makes more sense this way).
If you tell me how, I can prepare a new PR for 3.2 with this change. BC is 100% unchanged.

[ticket/14938] Inconsistency in ext_mgr all_available vs is_available
Made is_available much more strict, in line with the checks in all_available
Refactor all_available to use is_available, saving duplicate code.
Further simplify is_available by using metadata_manager.

PHPBB3-14938
@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

Well, I have incorporated the above mentioned code change, using metadata_manager to check for extension availability. For 3.1, there is an extra call to get the template from the container, that may be removed in 3.2.

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

The errros are due to a "Circular Dependency Injection" issue between ext.manager and template.
So, this method cannot be used for 3.1 (cannot get template object from container without creating the circular dependency).
To avoid the circular dependency in 3.1 we need to give default parameter values to template in the metadata_manager constructor interface.
The good news: the code for is_available is the same now for 3.1 and 3.2.

[ticket/14938] Inconsistency in ext_mgr all_available vs is_available
Made is_available much more strict, in line with the checks in all_available
Refactor all_available to use is_available, saving duplicate code.
Further simplify is_available by using metadata_manager.
Make optional the template object on metadata_manager creation.

PHPBB3-14938

@javiexin javiexin force-pushed the javiexin:ticket/14938 branch from 95ca766 to 8afd875 Feb 10, 2017

[ticket/14938] Inconsistency in ext_mgr all_available vs is_available
Made is_available much more strict, in line with the checks in all_available
Refactor all_available to use is_available, saving duplicate code.
Further simplify is_available by using metadata_manager.
Make optional the template object on metadata_manager creation.
Fix extension_manager_mock to have config and user components.

PHPBB3-14938

@javiexin javiexin force-pushed the javiexin:ticket/14938 branch from 8afd875 to 7b2ffaf Feb 10, 2017

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

There was an error due to the some objects not being instantiated in one of the tests, required for metadata_manager (passed as null). Not instantiated in the mock extension_manager object, now done.

The remaining error is a sniffer error, due to the use of a "default" value in an intermediate parameter. However, this is CORRECT, as it is NOT a default value, but a "type hint", as per the PHP manual:

To specify a type declaration, the type name should be added before the parameter name. The declaration can be made to accept NULL values if the default value of the parameter is set to NULL.

PHP Manual page

Strangely enough, this only kicks of in PHP 5.5 tests, not in 5.4 or in 5.6.

This is the affected line:
https://github.com/phpbb/phpbb/pull/4592/files#diff-c281ac320a01294180fa4225455f5687R79
public function __construct($ext_name, \phpbb\config\config $config, \phpbb\extension\manager $extension_manager, \phpbb\template\template $template = null, \phpbb\user $user, $phpbb_root_path)

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

Options to remove this error:

  1. Ignore it (maybe using @codingStandardsIgnore directives ?) - this is my preferred solution
  2. Add "default values" to subsequent parameters (in this case, $user = null and $phpbb_root_path = '' ) - could be an option, although it might be misinterpreted by users
  3. Remove the type-hinting for the template parameter - I do not like this too much
  4. Reorder the parameters to have template be the last - I would prefer NOT to do this

What should I do?

@javiexin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

The PR for ticket/15087 includes the 3.2 version of this PR.

marc1706 added a commit to marc1706/phpbb that referenced this pull request Apr 16, 2017
Merge pull request phpbb#4592 from javiexin/ticket/14938
[ticket/14938] Inconsistency in ext_mgr all_available vs is_available

@marc1706 marc1706 merged commit 7b2ffaf into phpbb:3.1.x Apr 16, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.