[Ticket/12508] phpbb(\extension)\finder should not depend on extension manager #2417
Conversation
|
I thought you wanted to rename extensions to components in the finder. |
| @@ -7,7 +7,7 @@ | |||
| * | |||
| */ | |||
|
|
|||
| namespace phpbb\extension; | |||
| namespace phpbb; | |||
|
|
|||
| /** | |||
| * The extension finder provides a simple way to locate files in active extensions | |||
There was a problem hiding this comment.
Should this comment be updated now?
The finder provides a simple way to locate files in the core and active extensions
|
@Nicofuma changing the word does not make too much sense, since we need to distinguish between core and extensions anyway. |
| if ($replace_list) | ||
| { | ||
| $this->extensions = array(); | ||
| } |
There was a problem hiding this comment.
Can you add a line break for readability? At first glance I though this was if/else.
|
Code looks good. |
| new \phpbb\filesystem(), | ||
| dirname(__FILE__) . '/', | ||
| new phpbb_mock_cache() | ||
| ); | ||
| $finder->set_extensions(array_keys($this->extension_manager->all_enabled())); |
There was a problem hiding this comment.
Why isn't this using get_finder on the manager?
There was a problem hiding this comment.
Because this is inside tests and we use a phpbb_mock_extension_manager
|
bump? |
|
Rebased onto develop-ascraeus, so this should be ready for another review |
|
I mentioned this on IRC before but can we please rename "set_extensions" to something like set_search_paths, and make the finder not have a reference to "extensions"? |
|
@naderman I guess we want to keep the |
|
@naderman fixed, can you take another look? |
1 similar comment
| * @param string $core_path The path relative to phpbb_root_path | ||
| * @return \phpbb\extension\finder This object for chaining calls | ||
| * @param array $components A list of components that should be searched | ||
| * @param bool $replace_list Should the list be emptied before adding the components |
There was a problem hiding this comment.
It's called set, not append. Remove this parameter, if you want to "append" or "set" components there should be two separate methods.
|
Missing param comments for the component option in a bunch of places, I still really don't like the architecture of this. You just renamed extensions to components for the finder. I think really the finder should just have to have a list of paths to search in, and not know anything about how components or core work etc. So maybe we should simply have one finder object contain a second core finder with different search paths? I mean you keep changing this around and putting a lot of work into it, I just don't see this improving the situation much? |
|
We should probably discuss what the real goal here is, before you continue? |
|
@naderman bump again |
PHPBB3-12508
PHPBB3-12508
PHPBB3-12508
|
Rebase and updated the CLI command from #2569 |
|
Working on this now :-) Will either replace with a new PR in a few hours or merge this, but one of those two is happening today! |
|
Alright just merging this for now. |
https://tracker.phpbb.com/browse/PHPBB3-12508