Bugfix/16243 show validation error when uri not unique

[i-just]

Description

Instead of disabling the element when uri validation fails, show the validation error so the user can action it.

Related issues

#16243

[samhibberd]

Hi @i-just @brandonkelly, I was just testing out these changes on my [4.x] install and something doesn't look right.

Previously if you duplicated (Save as a new...) an element you could change values and these would pull through to the new entry.

Although this fix does prevent an element from being duplicated in the case a uri is not unique, it does not allow you to change anything that would account for that, so changing a slug to something unique and then hitting "Save as a new..." results in the same Cannot duplicate flash notification.

I had expected (maybe i'm wrong) that the duplicated element would load in with any errors (inc uri ones) highlighted, that may not be by how craft works though (we do see this behaviour when duplicating products, but we do have a bunch of custom behaviors doing things to products so probably just us).

[i-just]

@samhibberd, do you have autosaveDrafts set to false by any chance?

[samhibberd]

Hi @i-just we do have it set false yes, I can't recall why, although I have tested with it set to true and it makes no difference.

I think with this change as it currently is, you can never duplicate an element, if it hasUris() and slug increment is disabled?

[i-just]

I can see the behaviour you described with autosaveDrafts set to false but not when it’s set to true (default).

When it’s set to true, the expected behaviour in v4 is: when you “Save as new”, and there’s an issue with the uri not being unique, the duplication error will show; you can then change, e.g. the slug, click “Save as new” again, and the changes you made will be saved to the new (enabled) element. Is that not what you’re seeing with the autosaveDrafts enabled?

(In v5, when you “Save as new”, regardless of whether there’s an issue with the uri not being unique, the element will be duplicated to an unpublished draft, and you’ll then get a validation error when you try to publish it, providing the uri is still not unique.)

[samhibberd]

@i-just you are right, this is the behaviour we see too (i was testing with a entry type that changes the enabled value in a before save event).

Is there a workaround for supporting 'Save as new" in this scenario with autosaveDrafts set to false?

[i-just]

Thanks for confirming! I’ll have a look into this for disabled autosaveDrafts.

[samhibberd]

Hi @i-just any luck with this?

[brandonkelly]

@samhibberd We discussed this today and decided to revert the change, as there wasn’t a clear answer for when autosaveDrafts is disabled (or when saving the duplicate very quickly, before a provisional draft had time to be created).

The behavior is better in Craft 5 though, where “Save as new entry” creates a new unpublished draft, which doesn’t yet have to worry about having a unique URI.

[samhibberd]

For us this is primarily about saving in the front end, where we don't have (or want to have) the concept of drafts or autosave, we just want to be able to create an element and make sure it has a unique uri, does this mean that we will be running into issues when trying to achieve this?

[brandonkelly]

If the URI format contains {slug}, then no, it’s not an issue, as Craft will automatically increment the slug until it results in a unique URI.

[samhibberd]

Hi @brandonkelly, that's exactly what we are trying to avoid by disabling the automatic slug incrementing (maxSlugIncrement => 1) and the source of this issue. From an SEO pov, we need clean unique urls for this particular section, and so need users and our staff to be prevented from creating anything that violates this rule.

I guess we can fallback on custom validation rules to prevent, but feels like the slug increment setting should have this covered?

[brandonkelly]

You could create a custom controller action that ensures the URI is going to be unique, and prevent form submit until a unique slug has been entered.

[samhibberd]

Ok, but if team members are using the cp no way to work around this, and it will only throw an error if someone tries to enable the element?

The maxSlugIncrement docs do suggest it throws an error, which is not the case in this scenario:

The highest number Craft will tack onto a slug in order to make it unique before giving up and throwing an error.

Am I right in thinking that this only impacts draft creation and duplicating? So in theory if we don't use / hide off the "Save as new..." option in the cp, then we are never going to run into this issue?

Another option could be to add something in the before or afterValidate methods for all elements that have uri's and detect any uri errors and add slug / or other errors to force prevent the save??

[brandonkelly]

The maxSlugIncrement docs do suggest it throws an error, which is not the case in this scenario:

Well, ElementHelper::setUniqueUri() does in fact throw an exception. It just ends up getting caught by ElementUriValidator:

cms/src/validators/ElementUriValidator.php

Lines 54 to 65 in 3a83de2

	try {
	Craft::$app->getElements()->setElementUri($model);
	} catch (OperationAbortedException) {
	// Not a big deal if the element isn't enabled yet
	if (
	$model->enabled &&
	$model->getEnabledForSite() &&
	!$model->getIsUnpublishedDraft()
	) {
	$this->addError($model, $attribute, Craft::t('app', 'Could not generate a unique URI based on the URI format.'));
	return;
	}

Am I right in thinking that this only impacts draft creation and duplicating? So in theory if we don't use / hide off the "Save as new..." option in the cp, then we are never going to run into this issue?

Correct.

Another option could be to add something in the before or afterValidate methods for all elements that have uri's and detect any uri errors and add slug / or other errors to force prevent the save??

The problem is that by the time we find out that the URI can’t be made unique, Craft is too far along in the duplication process to back out gracefully.

[samhibberd]

Ok thanks for the explanation, I took a good look over the ElementUriValidator before raising this issue, it is a bit confusing that it doesn’t look adhere to the scenarios set when it’s defined (as in the rules are set on DEFAULT, ESSENTIALS and LIVE scenarios, but it only sets an error if it's enabled). I understand why, but just felt counterintuitive.

We have written some logic to prevent duplication in other scenarios by just throwing an exception in the before save event, might be an option here.

One other thing to check, when duplicating in the cp, if you change the slug manually before Saving as new... it doesn't respect the new slug, is that right?

Also sorry for all the queries and questions, really trying to understand how this all works as it's pretty fundamental to a bunch of stuff we are working with.

[brandonkelly]

One other thing to check, when duplicating in the cp, if you change the slug manually before Saving as new... it doesn't respect the new slug, is that right?

Yeah, the entry is duplicated before any posted data is set on it.

[samhibberd]

Hi @brandonkelly just been doing some further work on this, is it right that the current logic does not allow data to be changed before duplicating, we see that behavior accross most elements.

However, and probably a big part of the confusion, is that if we duplicate a product prevents the save and displays the uri error inline. If we then manually change the posted data it allows the save.

This is the exact behaviour we would like to see in this scenario, is it due to products in commerce not supporting drafts? or something else?