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

Add language specific UUID URLs #6312

Draft
wants to merge 2 commits into
base: develop-minor
Choose a base branch
from

Conversation

bastianallgeier
Copy link
Member

This PR …

This is an attempt to finally get parts of the multi-language UUID issues solved. (#5551)

This adds a new route for language-specific UUID urls:

Examples:

/en/@/page/1234
/en/@/file/1234

Docs

Ready?

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

For review team

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

@distantnative
Copy link
Member

Either we should add unit tests or we should be more explicit what we want to ignore for coverage.

@bastianallgeier
Copy link
Member Author

@distantnative I'm trying to create tests for the UUID route but I fail to setup a test environment with working UUIDs. Could you maybe have a look at the test? Why can the model not be found like that?

@distantnative
Copy link
Member

Had you tried manually adding a uuid in the app setup ['content' => ['uuid' => 'my-uuid']]? That's how many of the other tests do it.

@bastianallgeier
Copy link
Member Author

Yep, I tried that too but also without luck.

@distantnative distantnative self-assigned this Mar 6, 2024
@bastianallgeier bastianallgeier added needs: help 🙏 Really needs some help on this needs: tests 🧪 Requires missing tests labels Mar 15, 2024
@distantnative
Copy link
Member

distantnative commented Mar 16, 2024

@bastianallgeier Ok I finally understood where it fails:

  • The uuid route looks up the model lazily, so only from cache. Since the cache is not populated, it fails
  • When I was trying to manually set a UUID, I did it with content. But we are in multilang territory, so we have to set the translations array instead

Alternatively, solving #5181 too.

@bastianallgeier bastianallgeier modified the milestones: 4.2.0, 4.2.x Mar 27, 2024
@distantnative distantnative modified the milestones: 4.3.x, 4.4.0 Jun 1, 2024
@distantnative distantnative marked this pull request as draft June 1, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: help 🙏 Really needs some help on this needs: tests 🧪 Requires missing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants