Problem/Motivation

In the dblog rest plugin, a message is given to the HttpException constructor, but the constructor actually takes a HTTP status code as the first parameter. This means that clients get an incorrect HTTP status code and no message explaining the situation.

The remaining uses of HttpException in core seem to be correctly used.

Proposed resolution

Fix the parameters; send a generic 400 status code.

Remaining tasks

Probably needs tests.
Needs review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#9 interdiff-2512668-7-9.txt842 bytesmartin107
#9 2512668-9.patch2.34 KBmartin107
#7 2512668-7.patch1.82 KBMattA
#5 2512668-5.patch1.86 KBMattA
#4 2512668-4.patch3.71 KBMattA
#3 2512668-3.patch1.82 KBMattA
#1 2512668.patch1.22 KBMattA

Comments

MattA’s picture

MattA commented
Issue summary: View changes
StatusFileSize
new 2512668.patch1.22 KB
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/dblog/src/Plugin/rest/resource/DBLogResource.php
@@ -49,6 +49,6 @@ public function get($id = NULL) {
-    throw new HttpException(t('No log entry ID was provided'));
+    throw new HttpException(400, t('No log entry ID was provided'));

What about using BadRequestHttpException directly?

MattA’s picture

MattA commented
StatusFileSize
new 2512668-3.patch1.82 KB

Exact same result, and I guess slightly more informative for developers?

MattA’s picture

MattA commented
Status: Active » Needs review
StatusFileSize
new 2512668-4.patch3.71 KB

Now with an easier than expected test.

MattA’s picture

MattA commented
StatusFileSize
new 2512668-5.patch1.86 KB

Messed up encoding in patch file.

Status: Needs review » Needs work

The last submitted patch, 5: 2512668-5.patch, failed testing.

MattA’s picture

MattA commented
StatusFileSize
new 2512668-7.patch1.82 KB

...and Windows sucks.

MattA’s picture

MattA commented
Status: Needs work » Needs review
martin107’s picture

martin107 commented
StatusFileSize
new 2512668-9.patch2.34 KB
new interdiff-2512668-7-9.txt842 bytes

In terms of review +1 from me ... the thrust of the argument is correct.

My patch fixes only a small nit ... the throwing of exceptions is not documented.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

That seems indeed more semantically correct

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed bb10bfa on 
    Issue #2512668 by MattA, martin107, dawehner: Dblog rest plugin throws...

  • catch committed a765d91 on 
    Issue #2512668 by MattA, martin107, dawehner: Dblog rest plugin throws...

Status: Fixed » Closed (fixed)

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