Refine routes list functionality #58

settermjd · 2024-11-14T11:55:44Z

Q	A
Documentation	yes
Bugfix	yes
BC Break	no
New Feature	no
RFC	no
QA	yes

Description

This PR implements all but one of the suggestions that @gsteel made in #52. Thank you for the motivation and inspiration to make the changes and improve the original work.

@gsteel

Previously, RoutesFilter was taking an array with the filter options (i.e., method, path, name, etc) but, as @gsteel noted, there was a perfectly good RouteFilterOptions class available. I don't know why I developed it, but overlooked it. This change refactors RoutesFilter to use it instead, then goes one step further and introduces a RouteFilterOptionsInterface so that, when no filter options are available, an EmptyRouteFilterOptions can be supplied, hopefully making the code easier to intuit and maintain. Signed-off-by: Matthew Setter <[email protected]>

Signed-off-by: Matthew Setter <[email protected]>

This change stores the filter options as member variables so that they're more readily reusable and so that it should be less likely to make a mistake with using them at some point in the future. Signed-off-by: Matthew Setter <[email protected]>

@gsteel

This change removes the class' default options, replacing them with unions of non-empty-string|null. In doing so it's explicit that null means the option is unset and should be ignored. This change was motivated by @gsteel's comment mezzio#52 (comment). I believe that I've implemented it correctly. Signed-off-by: Matthew Setter <[email protected]>

This is to ensure that any possible type of error is caught and handled. Signed-off-by: Matthew Setter <[email protected]>

I'm not sure why I didn't do this originally. Perhaps I was experimenting to learn how routing works in Mezzio more deeply. Regardless, this change refactors ListRoutesCommand to take a RouteCollector object instead of a ContainerInterface and from that ContainerInterface fetching a RouteCollector. Signed-off-by: Matthew Setter <[email protected]>

Signed-off-by: Matthew Setter <[email protected]>

This change refactors ListRoutesCommandFactory to retrieve a ConfigLoaderInterface object from the application's DiC instead of instantiating a RoutesFileConfigLoader object directly. The main reason for this is to provide flexibility to the user, should they have the need to retrieve routes in a different way than the default implementation, which searches in config/routes.php and in any routes registered through ConfigProvider classes. Signed-off-by: Matthew Setter <[email protected]>

This change provides a default implementation for retrieving an application's routes configuration. It uses the stock approach of searching for routes in config/routes.php and any registered with the Application object in any of the loaded ConfigProvider classes. It also provides a simplistic example for building a custom implementation, should it be required. Signed-off-by: Matthew Setter <[email protected]>

This change updates the packages ConfigProvider class to register a ConfigLoaderInterface service with the DI container provided by DefaultRoutesConfigLoaderFactory. If a user needs to override this implementation, they can do so as per the additional documentation in the package's README. Signed-off-by: Matthew Setter <[email protected]>

settermjd · 2024-11-14T12:00:28Z

I don't know if I've messed this PR up a little, given the number of commits. Also, the Psalm results I have locally are different from the GitHub check results. Would love some help getting them in sync.

alexmerlin · 2024-11-20T06:21:57Z

I don't know if I've messed this PR up a little, given the number of commits. Also, the Psalm results I have locally are different from the GitHub check results. Would love some help getting them in sync.

@settermjd Try running Psalm locally without cache:

vendor/bin/psalm --no-cache

Or explicitly clear cache first:

vendor/bin/psalm --clear-cache

before running Psalm as usually.

gsteel

Sorry @settermjd - I've got quite a lot of feedback here 😬

Overall, I think a lot of this can be simplified.

gsteel · 2024-11-14T13:18:38Z

src/ConfigProvider.php

@@ -66,6 +68,7 @@ public function getDependencies(): array
    {
        return [
            'factories' => [
+                ConfigLoaderInterface::class                    => DefaultRoutesConfigLoaderFactory::class,


The factory should be mapped to the implementation, i.e. RoutesFileLoader::class => Factory::class and the interface should be added to aliases, i.e. ConfigLoaderInterface::class => RoutesFileLoader::class,

Users then do the same thing with 'factories' => [MyLoader::class => MyFactory::class], 'aliases' => [ConfigLoaderInterface::class => MyLoader::class]

The difference is that I can ask the container for a specific implementation, or, I can ask it for the configured, default implementation by using the interface

gsteel · 2024-11-14T13:25:14Z

src/Routes/Filter/RouteFilterOptions.php

        if (is_array($methods)) {
+            array_walk($methods, fn(string &$value) => $value = strtoupper($value));


Suggested change

array_walk($methods, fn(string &$value) => $value = strtoupper($value));

$this->methods = array_map(static fn (string $value): string => strtoupper($value), $methods);

gsteel · 2024-11-14T13:30:10Z

src/Routes/Filter/RouteFilterOptions.php

+    /** @param string|null|array<array-key,string> $methods */
    public function __construct(
-        string $middleware = '',
-        string $name = '',
-        string $path = '',
-        $methods = []
+        private string|null $middleware,
+        private string|null $name,
+        private string|null $path,
+        private array|string|null $methods


I would drop null|string from $methods and make the argument list<string>, so you always have either list<string> or []

gsteel · 2024-11-14T13:38:37Z

src/Routes/Filter/RouteFilterOptionsInterface.php

+    /**
+     * Returns any route methods to filter the available routing data by
+     */
+    public function getMethods(): array|string|null;


Suggested change

/**

* Returns any route methods to filter the available routing data by

*/

public function getMethods(): array|string|null;

/**

* Returns any route methods to filter the available routing data by

*

* @return list<string>

*/

public function getMethods(): array;

gsteel · 2024-11-20T08:22:03Z

README.md

+
+By default, `Mezzio\Tooling\Routes\DefaultRoutesConfigLoaderFactory` registers a `ConfigLoaderInterface` service with the application's DI container, which retrieves the application's routes from two sources:
+
+- _config/routes.php_


First emphasis style on the page wins, so either change all the others to _ or adjust your changes to use *

The filename must be enclosed in backticks (`):

for a single file: filename.ext

for a file in a directory: directory/filename.ext

gsteel · 2024-11-20T10:52:24Z

src/Routes/ListRoutesCommand.php

-        ConfigLoaderInterface $configLoader
+        private RouteCollector $routeCollector,
+        private ConfigLoaderInterface $configLoader,
+        private RouteFilterOptionsInterface $filterOptions


The $filterOptions property is redundant - it's a value object created at runtime from a bunch of strings and it's immediately overwritten when calling execute

gsteel · 2024-11-20T10:53:39Z

src/Routes/Filter/EmptyRouteFilterOptions.php

@@ -0,0 +1,38 @@
+<?php


This class seems entirely uneccessary

gsteel · 2024-11-20T10:53:56Z

src/Routes/Filter/RouteFilterOptionsInterface.php

@@ -0,0 +1,35 @@
+<?php
+
+declare(strict_types=1);


This interface seems unnecessary

gsteel · 2024-11-20T10:59:31Z

test/Routes/ListRoutesCommandTest.php

-        $this->command         = new ListRoutesCommand($container, $configLoader);
+        $this->command         = new ListRoutesCommand(
+            $this->routeCollection,
+            $configLoader,
+            new EmptyRouteFilterOptions()
+        );


Symfony ships a 'CommandTester' which goes along the lines of:

$this->tester = new CommandTester($command);

You can then execute the command with a route collector instance that contains an array of actual routes and assert that the table output and json output match expectations rather than mocking everything and asserting a bunch of methods are called.

https://symfony.com/doc/current/console.html#testing-commands

If you make the route collector a test property, you can inject routes into it, per test to vary expected output based on options passed to the command and then there's effectively no mocking

gsteel · 2024-11-20T11:07:17Z

src/Routes/Filter/RouteFilterOptions.php

@@ -78,7 +75,11 @@ public function toArray(): array
    {
        $values = [];
        foreach (get_object_vars($this) as $key => $value) {
-            if (! empty($value)) {
+            if (in_array($key, $this->getFilterOptionsMinusMethods()) && $value !== "") {


The entire toArray() method is unused in the command AFAICT. Can it be dropped?

settermjd requested a review from gsteel November 14, 2024 11:55

settermjd self-assigned this Nov 14, 2024

settermjd force-pushed the refine-routes-list-functionality branch from 6baf56f to 49a82a8 Compare November 14, 2024 11:56

settermjd added 10 commits November 14, 2024 21:57

Add docblock comments for RouteFilterOptionsInterface methods

c1d7005

Signed-off-by: Matthew Setter <[email protected]>

Replace the Exception with a Throwable in RoutesFilter

5db68d1

This is to ensure that any possible type of error is caught and handled. Signed-off-by: Matthew Setter <[email protected]>

Minor code clean up

8ebec29

Signed-off-by: Matthew Setter <[email protected]>

settermjd force-pushed the refine-routes-list-functionality branch from 49a82a8 to 8629833 Compare November 14, 2024 11:58

gsteel mentioned this pull request Nov 17, 2024

Add support for PHP 8.4 #51

Closed

gsteel requested changes Nov 20, 2024

View reviewed changes

gsteel mentioned this pull request Nov 26, 2024

Add support for PHP 8.4 #59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine routes list functionality #58

Refine routes list functionality #58

settermjd commented Nov 14, 2024

settermjd commented Nov 14, 2024 •

edited

Loading

alexmerlin commented Nov 20, 2024

gsteel left a comment

gsteel Nov 14, 2024

gsteel Nov 14, 2024

gsteel Nov 14, 2024

gsteel Nov 14, 2024

gsteel Nov 20, 2024

froschdesign Nov 20, 2024

gsteel Nov 20, 2024

gsteel Nov 20, 2024

gsteel Nov 20, 2024

gsteel Nov 20, 2024

gsteel Nov 20, 2024

gsteel Nov 20, 2024

		if (is_array($methods)) {
		array_walk($methods, fn(string &$value) => $value = strtoupper($value));

	array_walk($methods, fn(string &$value) => $value = strtoupper($value));
	$this->methods = array_map(static fn (string $value): string => strtoupper($value), $methods);


		By default, `Mezzio\Tooling\Routes\DefaultRoutesConfigLoaderFactory` registers a `ConfigLoaderInterface` service with the application's DI container, which retrieves the application's routes from two sources:

		- _config/routes.php_

		@@ -0,0 +1,35 @@
		<?php

		declare(strict_types=1);

Refine routes list functionality #58

Are you sure you want to change the base?

Refine routes list functionality #58

Conversation

settermjd commented Nov 14, 2024

Description

settermjd commented Nov 14, 2024 • edited Loading

alexmerlin commented Nov 20, 2024

gsteel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

settermjd commented Nov 14, 2024 •

edited

Loading