silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Unique indexes create server errors rather than form validation notices #10674

Open NightJar opened 1 year ago

NightJar commented 1 year ago

Affected Version

At least 4.11, possibly earlier versions (untested). This may have worked in previous versions, as the way mysqli errors are delivered has changed in PHP 8, which may be the cause of this. c.f. https://github.com/silverstripe/silverstripe-framework/pull/10570 for more detail.

Description

When marking a field as unique via the use of an index constraint, uncaught exceptions are thrown on a failed write due to a duplicate value. This causes an unfriendly system halt and "Server error" to be shown, as opposed to normal form feedback (e.g. in the CMS).

Steps to Reproduce

With both Framework & Admin installed: Make a DataObject with a title field & unique index on Title Make a ModelAdmin for that DataObject Create an instance of the DataObject via the ModelAdmin with the Title "test" Try to create a second instance of the DataObject via the ModelAdmin with the Title "test"

πŸ‘€ "server error" toast message with no further info on how to validate the form.

GuySartorelli commented 1 year ago

Just to be clear, you've mentioned mysqli in passing but haven't explicitly said whether this issue is explicitly related only to mysqli. So does this also happen with other connectors? Or only mysqli?

NightJar commented 1 year ago

does this also happen with other connectors?

I don't know. It is not my situation, other adaptors can be considered outside scope of this issue. MySQL is the default & only supported connector (afaik), which uses the PHP mysqli library.

If the intention is to throw the framework generic DatabaseException then it isn't really of concern - other adaptors should also throw them in a similar circumstance. How the framework handles that in various circumstances shouldn't matter on the underlying library.

My intention with the comment was to perhaps provide a little "starting context" on how this problem exists. Unfortunately it may have distracted from the core of the issue, which is that a unique index gives a very poor experience when handling conflicts. Both as a developer and an administrator - arguably this could have been an issue on the CMS repo, but I think the issue is more relevant to validation issues in general than to usage of the CMS. I.e. SilverStripe\ORM\ValidationException and maybe Forms in general rather than CMSAdmin::EditForm or low-level database adaptor logic.

I suppose one might reframe the issue as:

I see this as a gap in what the framework offers and thus should be extended to include.

I hope that clarifies :)