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

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

bastianallgeier
Copy link
Member

@bastianallgeier bastianallgeier commented Feb 29, 2024

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 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
@distantnative distantnative force-pushed the feature/language-uuid-urls branch from 6915ab0 to 8f363c7 Compare July 24, 2024 09:04
@distantnative
Copy link
Member

@bastianallgeier Apparently, sometimes it takes months to realize what one is overlooking. I even wrote

Since the cache is not populated, it fails.

But only today it clicked that, well yea then we need to add a $uuids->populate() line to add it to the cache.

@distantnative distantnative force-pushed the feature/language-uuid-urls branch from b124c32 to 12b4e4f Compare July 24, 2024 09:11
@distantnative distantnative added type: bug 🐛 Is a bug; fixes a bug and removed needs: help 🙏 Really needs some help on this needs: tests 🧪 Requires missing tests labels Jul 24, 2024
@distantnative distantnative marked this pull request as ready for review July 24, 2024 09:13
distantnative
distantnative previously approved these changes Jul 24, 2024
Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

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

@bastianallgeier Please check if the unit test is testing the right things from your perspective.

@bastianallgeier
Copy link
Member Author

@distantnative yes, looks good to me! I can't believe that we might actually be able to merge this :)

@distantnative distantnative merged commit 4147b7e into develop-minor Jul 25, 2024
12 checks passed
@distantnative distantnative deleted the feature/language-uuid-urls branch July 25, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants