Follow-up to #2472323: Move modal / dialog to query parameters

Problem/Motivation

Currently, DefaultExceptionSubscriber returns a JSON object with a single property, "error".

If, however, your response is caught by the JSON subscriber, it returns a JSON object with a
single property, "message". This is very inconsistent and unpredictable for consumers, as they don't know where to look for the detail of the error. (The property name changes depending on the severity and specificity of the problem, which is very non-obvious.)

Proposed resolution

Standardize on "message', because that is more widely used at the moment and so the less invasive change.

Remaining tasks

Commit it.

User interface changes

None.

API changes

Debatable; if we view the name of that property as an API, it's a minor change. If we don't, then there's no API change.

Crell says it's not an API change. :-)

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it's an error for us to be inconsistent in our errors.
Issue priority Normal; there is only a small change with limited impact.
Disruption Should have no impact on anyone, except those who are relying on the format of an error message for REST responses. (Very very few people.)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Needs review » Active
lostkangaroo’s picture

Priority: Major » Normal
Status: Active » Needs review
FileSize
791 bytes

I kept looking for something to make this complicated and seemed to only find one instance where error is used other than message. Seems that we can standardize on message pretty easily.

Status: Needs review » Needs work

The last submitted patch, 2: standardize_error-2486943-2.patch, failed testing.

lostkangaroo’s picture

FileSize
2.75 KB
1.98 KB

This takes care of the tests that failed due to the text of the property being changed.

lostkangaroo’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Message it is. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: standardize_error-2486943-4.patch, failed testing.

lostkangaroo’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
1.65 KB

Quick reroll to fix the non apply. One test was fixed in the parent.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

wfm as well.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The issue summary doesn't seem to reflect the current state of the patch. I also don't quite grok why we're changing something that seems clearly an error into a non-error-type message; could we explain this in more detail? Thanks!

Crell’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Updated summary.

xjm’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Thanks @Crell! The updated summary helps. I tweaked things a little (setting it to bug in accordance with what the summary says, and removing something about it possibly being major which didn't seem at all major anyway.) :)

I still don't entirely get why we would standardize on message (which seems semantically wrong) over error (which seems semantically correct) to minimize the disruption of something that (per the summary) is already not disruptive, but if @Crell and @neclimdul agree, well that's good enough for me. (And @Crell is the component maintainer, so that signoff helps reassure as well.) I reviewed with git diff --color-words and confirmed that the conversion of error to message in code and test is the only change.

I considered whether this should have a change record, but based on the summary it doesn't seem so.

This issue is a normal bug fix, and doesn't include any very disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.

  • xjm committed e6ea417 on 8.0.x
    Issue #2486943 by lostkangaroo, Crell, neclimdul: Standardize error...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.