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

[Sluggable] Use TranslationWalker for Translatable objects when looking for similar slugs #2620

Merged

Conversation

ambroisemaupate
Copy link
Contributor

Force translation walker to look for slug translations to avoid duplicated slugs.


Fixes #100, fixes #2530

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2023

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.39%. Comparing base (0632ab1) to head (43809dc).
Report is 27 commits behind head on main.

Files Patch % Lines
src/Sluggable/Mapping/Driver/Annotation.php 60.00% 2 Missing ⚠️
src/Mapping/Annotation/Slug.php 50.00% 1 Missing ⚠️
src/Sluggable/Mapping/Driver/Xml.php 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2620      +/-   ##
==========================================
- Coverage   78.75%   78.39%   -0.36%     
==========================================
  Files         163      167       +4     
  Lines        8593     8613      +20     
==========================================
- Hits         6767     6752      -15     
- Misses       1826     1861      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ambroisemaupate
Copy link
Contributor Author

Changelog and Unit test have been added.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Even if this may be considered as a fix, I guess the change will break the behavior for users relying on the current behavior.

Is this behavior documented? If not, I think this PR must also provide the required documentation.

How we can build a strong and indisputable argument to support this change as a fix and not as a new feature?

I'd like to hear more opinions here.
/cc @mbabker, @franmomu.

@tharsanvn
Copy link

Hello guys :)

Any news about this PR ?

@mbabker
Copy link
Contributor

mbabker commented Sep 29, 2023

Even if this may be considered as a fix, I guess the change will break the behavior for users relying on the current behavior.

Is this behavior documented? If not, I think this PR must also provide the required documentation.

How we can build a strong and indisputable argument to support this change as a fix and not as a new feature?

I'd put it behind a config from the metadata instead of making a hard behavioral change. For the folks who want globally unique slugs, even across languages, they'd turn on this behavior with the config in the metadata.

If a platform is OK with duplicated slugs across languages, then it should also have something in place to deal with that potential duplication (i.e. if your route definition is /{locale}/{category}/{post}, then /en/tips/translation and /fr/tips/translation should already provide fully unique paths that allow querying the right info without requiring globally unique slugs for the category or post.).

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added Stale and removed Stale labels Mar 27, 2024
@ambroisemaupate
Copy link
Contributor Author

ambroisemaupate commented May 23, 2024

How would you implement this as configurable? @mbakker
Adding uniqueOverTranslations option in Gedmo\Slug attribute ?

@ambroisemaupate
Copy link
Contributor Author

@phansys @mbabker

I've added a new option uniqueOverTranslations (default: false) to trigger TranslationWalker::class ORM hint in getSimilarSlugs method.

CHANGELOG.md Outdated Show resolved Hide resolved
@franmomu franmomu closed this Jun 9, 2024
@franmomu franmomu reopened this Jun 9, 2024
@franmomu franmomu force-pushed the fix/sluggable-translatable branch from 36a408f to 1d135a4 Compare June 9, 2024 07:50
@franmomu franmomu merged commit 8657591 into doctrine-extensions:main Jun 9, 2024
16 of 18 checks passed
@franmomu
Copy link
Collaborator

franmomu commented Jun 9, 2024

thanks @ambroisemaupate for keeping working on this! and @mbabker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants