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

Preview tokens 3: Preview URL refactoring #6823

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Dec 1, 2024

🚨 Merge first

Description

Summary of changes

A PR with three related refactoring changes to the existing preview URL logic.

Reasoning

  • Duplicated code in $page->previewUrl() and $site->previewUrl() would become even more in following PRs if we wouldn't DRY it
  • v5 is a good opportunity to rename the token query param in draft preview URLs to _token as the generation logic of those tokens will change anyway (in PR 8)

Additional context

$page->previewUrl() and $site->previewUrl() can stay as aliases/shortcuts as they do no harm and might be useful to devs as well.

Changelog

Housekeeping

  • Consolidate duplicated logic from $page->previewUrl() and $site->previewUrl() in $version->url() (with aliases to the existing methods)

Breaking changes

  • Rename token query param for preview URLs to _token for consistency with the new _version param

Breaking changes (since alphas)

  • Rename parameter name $page->previewUrl($version) to $page->previewUrl($versionId) for consistency with other methods

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@lukasbestle lukasbestle added the type: refactoring ♻️ Is about bad code; cleans up code label Dec 1, 2024
@lukasbestle lukasbestle added this to the 5.0.0-beta.1 milestone Dec 1, 2024
@lukasbestle lukasbestle self-assigned this Dec 1, 2024
@lukasbestle
Copy link
Member Author

Coverage seems to be another PHPUnit issue. If I rename Version::url() to another name, the coverage is detected properly.

@distantnative
Copy link
Member

@lukasbestle Had that problem with Uuid::url() before, really seems to be that method name.

Base automatically changed from v5/changes/preview-tokens-2 to v5/develop December 2, 2024 08:07
Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

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

Nice! The new solution with Version::url() works really well.

@bastianallgeier bastianallgeier merged commit 55125ed into v5/develop Dec 2, 2024
10 of 11 checks passed
@bastianallgeier bastianallgeier deleted the v5/changes/preview-tokens-3 branch December 2, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants