[ticket/11465] Use extension finder when adding extensions' acp modules #1313

Merged
merged 12 commits into from May 12, 2013

Conversation

Projects
None yet
5 participants
Owner

marc1706 commented Mar 21, 2013

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

[ticket/11465] Use extension finder when adding extensions' acp modules
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
Contributor

EXreaction commented Mar 21, 2013

Seeing as there were two other bugs just fixed in this code
http://tracker.phpbb.com/browse/PHPBB3-11371
http://tracker.phpbb.com/browse/PHPBB3-11395

I think we ought to have some unit tests for this function.

Owner

marc1706 commented Mar 21, 2013

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.

Contributor

EXreaction commented Mar 22, 2013

Don't try to make functional tests, just unit test the function.

[ticket/11465] Add unit tests for acp_modules::get_module_infos()
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
Owner

marc1706 commented Mar 22, 2013

Done. :D

tests/extension/modules_test.php
+ {
+ // the modules' info files needs to be included before the test for
+ // some reason ...
+ require_once dirname(__FILE__) . '/ext/foo/acp/a_info.php';
@nickvergessen

nickvergessen Mar 22, 2013

Contributor

requires should be at the top of the file (see other tests)

Owner

naderman commented Mar 31, 2013

Please add another unit test that checks that the old style module files are still found correctly.

Owner

marc1706 commented Apr 4, 2013

Will do.

Contributor

nickvergessen commented Apr 21, 2013

Tests pass now, was unable to get the "segmentation fault" away

@@ -0,0 +1,16 @@
+<?php
+
+class phpbb_ext_foo_acp_foo_info
@p

p Apr 29, 2013

Contributor

File name is "fail" rather than "foo". If this is intentional, comment should be added explaining why.

@marc1706

marc1706 May 10, 2013

Owner

It is intentional. I'll add a comment to the file.

+ 'config' => array('title' => 'Config', 'auth' => '', 'cat' => array('MCP_MAIN')),
+ ),
+ ),
+ ), $acp_modules);
@p

p Apr 29, 2013

Contributor

Please add more comments as to what is being tested.

Owner

marc1706 commented May 10, 2013

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
@nickvergessen

nickvergessen May 10, 2013

Contributor

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

Contributor

EXreaction commented May 12, 2013

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.

[ticket/11465] Add tests for optional arguments of get_module_infos()
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
Owner

marc1706 commented May 12, 2013

@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.

Owner

marc1706 commented May 12, 2013

You also might want to restart the tests as travis seems to be going nuts today.

Contributor

EXreaction commented May 12, 2013

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.

[ticket/11465] Add disabled ext to allow proper testing of get_module…
…_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
Owner

marc1706 commented May 12, 2013

@EXreaction better? :D

marc1706 added some commits May 12, 2013

[ticket/11465] Increase count of disabled extensions to 5 in function…
…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
Owner

marc1706 commented May 12, 2013

Failing functional test should be fixed now ...

@EXreaction EXreaction merged commit bd6cebf into phpbb:develop May 12, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
Contributor

EXreaction commented May 12, 2013

This looks great now, thanks!

@marc1706 marc1706 deleted the marc1706:ticket/11465 branch Jan 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment