Extract a new REST controller from core `Modules`

[aaemnnosttv]

Feature Description

The core Modules class handles more than it should right now, with a large portion of its body devoted to its REST endpoints. The REST-specific functionality it provides should be extracted into a new controller class, similar to others, leaving the Modules class dedicated to only core modules logic and behavior.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• All rest routes registered and related helper methods and defined by the core Modules class should be extracted to a new Google\Site_Kit\Core\Modules\REST_Modules_Controller class
  • An accompanying test case should be created for it, similar to others that exercises the REST endpoints it registers and all REST-related tests should move from ModulesTest to the controller's test case
  • Test coverage should be added for any untested endpoints (note sharing-settings has its own controller, but these could be merged later when the feature flag is removed)
  • The controller should be registered within Modules::register
• No change in behavior should be made/observable here

Implementation Brief

Create a new REST_Modules_Controller class in a new file includes/Core/Modules/REST_Modules_Controller.php with the following methods:

• __construct - set the Modules instance to a class property
• register - Move the googlesitekit_rest_routes and googlesitekit_apifetch_preload_paths filter hooks to register method.
• Move the get_rest_routes method from Modules class to REST_Modules_Controller.
• Move the following REST endpoints-related helper methods:
  • get_module_schema,
  • prepare_module_data_for_response
  • And any other private/protected helper methods that can be moved.
• In the Modules class, register the REST_Modules_Controller::register method in the register method.

Test Coverage

• Create a new test file tests/phpunit/integration/Core/Modules/REST_Modules_ControllerTest.php.
• Move all the test cases for the above methods, which were being moved to the new controller class.
• Add new test cases for the methods in the controller class that doesn't have any test cases.

QA Brief

• There should be no visual regressions following this change.
• Setup SiteKit on a new site and ensure everything works fine, from setting up modules to modules grabbing data.

Changelog entry

[aaemnnosttv]

Thanks @hussain-t – this is almost there, just a few points.

Move the following REST endpoints-related helper methods:

• get_module_dependencies,
• get_module_schema,
• prepare_module_data_for_response
• is_module_connected
• And any other helper methods that can be moved.

Any public methods of Modules shouldn't (and wouldn't have to be) moved if you have an instance of the class in the controller, they would be callable on the instance.

Only protected and private methods can safely be moved.

It's worth noting that googlesitekit_apifetch_preload_paths is hooked in controller classes as well, so this would be good to move as well.

Also, this is a substantial change so let's give it a little more time with the estimate to allow more time for tests.

[hussain-t]

Thanks, @aaemnnosttv. I have updated the IB and increased the estimate.

[aaemnnosttv]

Thanks @hussain-t

IB ✅

[mohitwp]

QA Update ✅

• Verified on dev.
• Tested site kit setup.
• Tested set up of all modules.
• Tested all widget's data.
• Tested disconnect/reset functionality.
• @asvinb Looking at the AC. I feel that it should get verify by QA:eng.

cc @wpdarren

[jimmymadon]

QA:Eng ⚠️

❗ On checking PHPUnit code coverage, we've covered 89% of lines for the get_rest_routes() method which is great! The only endpoint not covered for successful cases is the 'modules/(?P<slug>[a-z0-9\-]+)/data/notifications'. I don't think this is a big issue but any particular reason this was skipped? ✅ Performed a quick smoke test of the plugin. ✅ Verified all REST endpoints have been moved over and are covered with tests barring the one mentioned above.

(Note: Haven't gone through all the code again in detail due to the large size of changes and I can see that @aaemnnosttv along with @nfmohit did extremely thorough reviews here.)

Screenshots

[aaemnnosttv]

❗ On checking PHPUnit code coverage, we've covered 89% of lines for the get_rest_routes() method which is great! The only endpoint not covered for successful cases is the 'modules/(?P<slug>[a-z0-9\-]+)/data/notifications'. I don't think this is a big issue but any particular reason this was skipped?

@jimmymadon thanks for raising this. I'm guessing that part of this endpoint was probably skipped because it goes through Module::get_data which we generally have very little coverage for so this is consistent with that. AdSense is the only module that implements that datapoint but we could test it with a test-specific module instance to avoid trying to mock the Google API/client. I don't think we need to fail the QA on this alone but it would be good to address in a follow up.

[jimmymadon]

QA: Eng ✅

@aaemnnosttv Thanks for that clarification. Sounds good to me!