[ticket/11465] Use extension finder when adding extensions' acp modules #1313
Conversation
The method acp_modules::get_module_infos() needs to use the extension finder whenever it is looking for a module's info file. While transitioning to the new extension system, only the initial search for all module info files was changed to the new system. Due to this it is not possible to add an extension's acp/mcp/ucp module manually in the ACP. This patch will always use the extension finder for the acp module's info files and therefore properly find the needed file. Additionally, the code has been cleaned up a little bit. PHPBB3-11465
|
Seeing as there were two other bugs just fixed in this code I think we ought to have some unit tests for this function. |
|
I'm currently tryingt to add unit tests for that function but I'm really having trouble getting an acp module to work from unit tests. |
|
Don't try to make functional tests, just unit test the function. |
The tests add 3 different modules. One acp module that should be found (acp/a_module), one acp module that should not be found (acp/fail_module), and one mcp module that should work again (mcp/a_module). The modules' info files had to be included as they were not auto-loaded for some reason. There are several test stages. First of, it is tested if the correct mcp and acp module is returned. Afterwards, the proper loading of specified modules is tested. One with an existing module and one with a not existing module. Finally, the test concludes with trying to get the module info of not existing ucp modules. Other classes like foobar would have also worked for that check but I decided to use the ucp type of class as that is the one type missing from the added test modules. PHPBB3-11465
|
Done. :D |
| { | ||
| // the modules' info files needs to be included before the test for | ||
| // some reason ... | ||
| require_once dirname(__FILE__) . '/ext/foo/acp/a_info.php'; |
There was a problem hiding this comment.
requires should be at the top of the file (see other tests)
|
Please add another unit test that checks that the old style module files are still found correctly. |
|
Will do. |
|
Tests pass now, was unable to get the "segmentation fault" away |
| @@ -0,0 +1,16 @@ | |||
| <?php | |||
|
|
|||
| class phpbb_ext_foo_acp_foo_info | |||
There was a problem hiding this comment.
File name is "fail" rather than "foo". If this is intentional, comment should be added explaining why.
There was a problem hiding this comment.
It is intentional. I'll add a comment to the file.
|
I have added some more commits that should explain the tests properly. |
| * Due to the mismatch between the class name and the file name, this module | ||
| * file shouldn't be found by the extension finder | ||
| */ | ||
| class phpbb_ext_foo_acp_fail_module |
There was a problem hiding this comment.
The class name matches the file name here, so you should modify this the class name and the file name of the info file to the
PHPBB3-11465
|
Please expand the unit tests a bit to test that the parameters to get_module_infos are all returning expected output, i.e. add a test for get_module_infos('phpbb_ext_foo_acp_a_module') and make sure it returns the correct result and test other combinations of input. If all options are tested I'm good with merging this. |
The possibilities of the first argument have already been covered previously. The second argument will be covered with an entry that should exist, an incorrect entry, and the default false entry that should use the previously set module class. Unfortunately, the third argument doesn't have an effect in the tests, as the mocked extension manager will not properly handle enabled/disabled extensions. PHPBB3-11465
|
@EXreaction The added tests should cover all the bases. The third parameter to the get_module_infos() function unfortunately doesn't do anything in the unit tests as the extension manager doesn't seem to support enabled/disabled extensions. If it would, one could add a disabled extension and check if changing the third parameter to true will cause it to all display a disabled extension's module info files. |
|
You also might want to restart the tests as travis seems to be going nuts today. |
|
The mock extension manager inherits from the extension manager from what I'm seeing. To test the third parameter, you'd just put an extension in the ext/ directory, but not enable it, then sending false as the third parameter should not return it, while true should. The reason that parameter exists is for the extension installation process, extensions are not enabled until they are completely installed, but to install them we need to find the modules included, so we need to make sure this works properly as well. |
…_infos() This will now also enable us to test the $use_all_available parameter of get_module_infos(), which will not only return the module infos for enabled extensions but also those from disabled extensions. PHPBB3-11465
|
@EXreaction better? :D |
…al test The ACP function test checks the amount of disabled extensions. Due to the added disabled extension for the tests of the acp_modules method get_module_infos(), this needed to be increased from 4 to 5. PHPBB3-11465
|
Failing functional test should be fixed now ... |
|
This looks great now, thanks! |
The method acp_modules::get_module_infos() needs to use the extension
finder whenever it is looking for a module's info file. While
transitioning to the new extension system, only the initial search for all
module info files was changed to the new system. Due to this it is not
possible to add an extension's acp/mcp/ucp module manually in the ACP.
This patch will always use the extension finder for the acp module's info
files and therefore properly find the needed file. Additionally, the code
has been cleaned up a little bit.
It's missing some tests but I'm currently not sure how we'd be able to achieve that. I'm pretty sure that it's not possible to add a functional test with an extension that is inside the tests folder, or is it?
Ticket: http://tracker.phpbb.com/browse/PHPBB3-11465
PHPBB3-11465