Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[3.0]: Add integrate_build_route hook #8474

Merged

Conversation

dragomano
Copy link
Contributor

While testing the ability to add new routes, I discovered that there is no way to add routes for URLs like /index.php?page=somepage and similar ones. This hook provides that capability.

Signed-off-by: Bugo <cheetah@bk.ru>
Signed-off-by: Bugo <cheetah@bk.ru>
@Sesquipedalian
Copy link
Member

Sesquipedalian commented Feb 15, 2025

Yes there is. When integrate_parse_route is called, which is very early in the process, you add your routable class to QueeyString::$route_parsers. Then when the time comes to build the route, which is very late in the process, your routable class is already in the list and will be called upon to build the route.

@Sesquipedalian
Copy link
Member

Perhaps I should change the name of the hook to make it clearer that it is for adding route parsers in general, not just for parsing routes specifically.

@Sesquipedalian Sesquipedalian merged commit 8fed87b into SimpleMachines:release-3.0 Feb 15, 2025
6 checks passed
@Sesquipedalian
Copy link
Member

FYI, @dragomano, I reverted 20f64f3 and then added 65fef3f instead. You can simply use integrate_route_parsers to add a class that implements SMF\Routable to QueryString::$route_parsers. That will ensure that your class that implements SMF\Routable is available both for building routes and parsing routes when needed.

@dragomano
Copy link
Contributor Author

Something isn't working. In my version, I could use the hooks like this:

	public function buildRoute(&$route_base, $params): void
	{
		if (isset($params['page'])) {
			$route_base = 'pages';
		}
	}

	public function parseRoute(): void
	{
		QueryString::$route_parsers['pages'] = Page::class;
	}

after which I would get the desired transformation: /index.php?page=smithchester => /pages/smithchester

class Page implements Routable
{
	use ResponseTrait;

	public static function buildRoute(array $params): array
	{
		$route = [];

		if (isset($params['page'])) {
			$route[] = 'pages';
			$route[] = $params['page'];

			unset($params['page']);
		}

		return ['route' => $route, 'params' => $params];
	}

	public static function parseRoute(array $route, array $params = []): array
	{
		array_shift($route);

		// We need to redirect from `/pages` to `/`
		if (empty($route)) {
			(new Response())->redirect();
		}

		$params['page'] = array_shift($route);

		return $params;
	}
}

@@ -27,7 +27,7 @@ trait ActionTrait
* An instance of this class.
* This is used by the load() method to prevent multiple instantiations.
*/
protected static self $obj;
protected static $obj;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPStorm says Property type does not match because static self and @var static are not the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I would have said correct the @var as the documentation above says it should be an instance of the class.

@Sesquipedalian
Copy link
Member

@dragomano, what are you doing in the function that hooks into integrate_route_parsers?

It should look similar to this:

function add_route_parser(): void
{
	SMF\QueryString::$route_parsers['page'] = SMF\Page::class;
}

@dragomano
Copy link
Contributor Author

@dragomano, what are you doing in the function that hooks into integrate_route_parsers?

It should look similar to this:

function add_route_parser(): void
{
	SMF\QueryString::$route_parsers['page'] = SMF\Page::class;
}

I need to use QueryString::$route_parsers['pages'] = Page::class;, but id doesn't help. And QueryString::$route_parsers['page'] = Page::class; doesn't work too. I'm interested in the following replacement: /index.php?page=slug => /pages/slug

@dragomano
Copy link
Contributor Author

Once again, the following code is currently used in QueryString.php:

		if (isset($params['action'])) {
			$route_base = $params['action'];
		} elseif (isset($params['board'])) {
			$route_base = 'boards';
		} elseif (isset($params['topic'])) {
			$route_base = 'topics';
		} elseif (isset($params['msg'])) {
			$route_base = 'msgs';
		}

It works for actions, and boards, and topics, and msgs, but I need to add this case:

		} elseif (isset($params['page'])) {
			$route_base = 'pages';
		}

@Sesquipedalian
Copy link
Member

I understand now. Yes, that needs to be addressed.

@Sesquipedalian
Copy link
Member

#8477 has reapplied commit 20f64f3.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants