Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

refactor(head-analytics): migrate php code to an action, add coverage #2974

Merged

Conversation

wescopeland
Copy link
Member

This PR migrates the URL path sanitization and props extraction code in head-analytics.blade.php to a reusable action that is also covered by tests.

Currently, there is an issue in Plausible where our self-healing system URLs are not being sanitized correctly:
Screenshot 2024-12-23 at 1 25 35 PM

This will be a recurring problem going forward as more URLs use slugs. The changes in this PR address the issue:
Screenshot 2024-12-23 at 1 26 25 PM

How to test this PR
Open your network tab and navigate to various pages. Check the "event" call. The p and u parameters are the ones of interest.

@wescopeland wescopeland requested a review from a team December 23, 2024 18:27
Comment on lines 31 to 47
public function __construct()
{
// Routes that support self-healing URLs (or will in the future) with ID lookups.
$this->addModelRoute('game', Game::class, 'title');
$this->addModelRoute('achievement', Achievement::class, 'title');
$this->addModelRoute('hub', GameSet::class, 'title');
$this->addModelRoute('system', System::class, 'name');

// Routes that just use a string parameter.
$this->addStringRoute('user', 'username');

// Routes that just use an ID.
$this->addIdRoute('ticket');

// Routes with nested ID-slug segments.
// TODO $this->addNestedRoute('forums', ['category', 'forum']);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to make this as simple as possible for adding new routes. Nested routes are not yet implemented, but some TODOs are in place.

Ideally a developer should not need to write regex to add model-bound routes. It should be a fairly trivial thing.

@wescopeland wescopeland merged commit 6ec695d into RetroAchievements:master Jan 1, 2025
8 checks passed
@wescopeland wescopeland deleted the head-analytics-action branch January 1, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants