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

Allow duplicate entry slugs #3671

Merged
merged 28 commits into from Jun 7, 2021
Merged

Allow duplicate entry slugs #3671

merged 28 commits into from Jun 7, 2021

Conversation

jasonvarga
Copy link
Member

@jasonvarga jasonvarga commented May 7, 2021

Fixes #714

Filename suffixes

You can now have two (or more) entries with the same slug and date combination by appending a suffix.

The suffix is a dot, then anything:

post.md
post.1.md
post.2.md

or with dates:

2021-05-10.post.md
2021-05-10.post.1.md
2021-05-10.post.2.md

The suffix can be anything if you're creating files by hand, but when using the control panel or creating entries programmatically, it'll use incrementing numbers. (It checks from zero though, so if you have .1 and .3, it'll make a 2, not a 4.)

URI Validation

Even though slugs no longer need to be unique, the URIs need to be.

Entries now get duplicate URI validation. The message will be shown on the slug field.

image

When you submit a tree, it'll check whether there are any duplicate URIs and prevent saving.

Screen Capture on 2021-05-28 at 17-57-20

Backwards compatibility

Technically, the removal of unique slug validation could be considered a breaking change, so this PR includes an update script that'll add unique slug validation to all collection blueprints.

If you actually wanted unique slugs, the automatically added rule will keep this behavior.
If you don't want the slugs to be unique, then you can remove the rule that gets added.

image

Deprecations

Entry::findBySlug() has been deprecated. Since the concept of a slug is no longer really unique, finding by one makes even less sense. You could have already had entries with duplicate slugs (if the dates were different) so it was already a little weird. We don't use this method anywhere.

Also deprecated Term::findBySlug() for method consistency. We don't use it anywhere either.

Todo

  • Stache should handle the filename suffix and don't treat it as part of the slug or date.
  • Saving an entry with a duplicate slug/date combo should append a suffix to the filename.
  • The uniqueness validation rule on slug should actually be uniqueness of the uri.
  • In a tree, prevent dragging an entry to where there's already an entry with that slug.
  • Update script to add unique validation rule to slug fields to keep existing sites working the same way.
  • Translate the duplicate URI validation message
  • Adjust the version in AddUniqueSlugValidation::shouldUpdate, if necessary.

@goellner
Copy link
Contributor

sorry to be this guy, but is this planned to be released in the next two weeks by any chance? have to launch a client site and would very much prefer to be able to use same slugs feature. if thats not realistic, no problem, I will need to build a workaround for the time being.

@jasonvarga
Copy link
Member Author

Unless something else important pops up, yes that's the plan.

@jasonvarga
Copy link
Member Author

Add a way to see a uri column in the entry listing. The column picker only shows blueprint fields, and URI isn't a field.

Tried to solve this, but turned into a bigger can of worms. We're going to ship this feature without a URI column for now, but circle back to this shortly. It's not a big deal yet because if you're going to have duplicate slugs, you're probably going to be viewing the collection in tree view.

@jasonvarga jasonvarga changed the title Allow duplicate slugs Allow duplicate entry slugs Jun 4, 2021
@jasonvarga jasonvarga marked this pull request as ready for review June 4, 2021 19:17
@jasonvarga jasonvarga merged commit 5f0f5c4 into 3.1 Jun 7, 2021
@jasonvarga jasonvarga deleted the feature/duplicate-slugs branch June 7, 2021 20:50
@goellner
Copy link
Contributor

goellner commented Jun 8, 2021

I am getting errors, when trying to use the same slug on two different sites. The weird part: It shows two different error messages on two pages in the same collection.

One is This value has already been taken and the other one This URI has already been taken.

I have two sites set up default and en and can't save the en content, because of those two errors. This happens in my pages collection and the routing is set up like this: {parent_uri}/{slug}

When I log the $uri here: https://share.getcloudapp.com/lluNyxWL it logs the same URI for both sites. My multisite setup is with folders asdf.com/test and asdf.com/en/test. I think the URI of de en locale should have the /en part in.

@goellner
Copy link
Contributor

goellner commented Jun 8, 2021

When you change the slug of the en version, statamic saves the entry. But after a reload the slug is the same as in the default locale. It seems atm I can't change the slug of the second locale at all.

@goellner
Copy link
Contributor

goellner commented Jun 8, 2021

I just tested the PR #3808 and while I can save with the same slug, I can't change the slug in either locale anymore. After the save and a reload the old value is shown

@jasonvarga
Copy link
Member Author

When you upgrade, we keep the unique slug validation. Did a new validate rule get added to the slug field your blueprint? You can remove it.

@goellner
Copy link
Contributor

goellner commented Jun 8, 2021

I removed that rule, but couldn’t change the slug anymore. Reverted back to the previous version. Also had to rename the files manually. I have one page with the same slug in the root of the tree. Couldn’t manage to get Statamic to add the number to the filename

@jasonvarga
Copy link
Member Author

That was hard to follow. I'm going to merge #3808 and release it. You can open a new issue if you still have trouble.

@rrelmy
Copy link
Contributor

rrelmy commented Jun 9, 2021

In this PR \Statamic\Facades\Term::findBySlug($slug, $taxonomy) has been deprecated but not alternative was document.

I am confused how I should load terms when I only have the slug from the yaml.

@goellner
Copy link
Contributor

goellner commented Jun 9, 2021

In this PR \Statamic\Facades\Term::findBySlug($slug, $taxonomy) has been deprecated but not alternative was document.

I am confused how I should load terms when I only have the slug from the yaml.

Using Term::findBySlug myself, there is no alternative afaik

@jasonvarga
Copy link
Member Author

jasonvarga commented Jun 9, 2021

Term::query()
            ->where('slug', $slug)
            ->where('taxonomy', $taxonomy)
            ->first();

@rrelmy
Copy link
Contributor

rrelmy commented Jun 9, 2021

Makes sense 🤦

Why can't we just keep the findBySlug for terms?
I understand it makes no sense for entries, but for terms it is still valid.

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.

Handle same slugs at different positions in a structure
3 participants