Problem/Motivation

POSTing to ie /entity/node with empty header/content gives or wrong header gives no clue about what was send.

The admin/reports/dblog shows an exception but the REST client is not informed except for a "415 Unsupported Media Type" response

No route found that matches the Content-Type header

How to test

curl --header "Content-Type: WRONG" --data '' http://drupal.d8/entity/node

Proposed resolution

Remaining tasks

User interface changes

API changes

Comments

clemens.tolboom’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, core-use-values-rest-exceptions.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
StatusFileSize
new1.81 KB

Added String import

Status: Needs review » Needs work

The last submitted patch, 3: inform_user_with-2417917-3.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
StatusFileSize
new2.64 KB

Status: Needs review » Needs work

The last submitted patch, 5: inform_user_with-2417917-5.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
StatusFileSize
new2.64 KB

Darn .

Status: Needs review » Needs work

The last submitted patch, 7: inform_user_with-2417917-7.patch, failed testing.

clemens.tolboom’s picture

clemens.tolboom’s picture

Status: Needs work » Needs review
StatusFileSize
new1.89 KB

Patch needed a reroll due to https://www.drupal.org/node/2405091.

I still don't get the failure in

 /**
   ...
   * @expectedException \Symfony\Component\HttpKernel\Exception\UnsupportedMediaTypeHttpException
   * @expectedExceptionMessage No route found that matches the Content-Type header application/hal+json.
   */
  public function testNoRouteFound() {
...

What is wrong with the expectedExceptionMessage?

Status: Needs review » Needs work

The last submitted patch, 10: inform_user_with-2417917-10.patch, failed testing.

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new1.92 KB

I completely missed $format was set to NULL so we need to use the 'raw' CONTENT_TYPE instead.

klausi’s picture

Title: Inform user with available values » Include content type format name in error response
Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
@@ -47,7 +48,7 @@ public function filter(RouteCollection $collection, Request $request) {
+    throw new UnsupportedMediaTypeHttpException(String::format('No route found that matches the Content-Type header @format.', array('@format' => $request->headers->get('CONTENT_TYPE'))));

I think the String class is gone, so we need to use SafeMarkup instead.

Otherwise makes sense to me.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI novice
StatusFileSize
new1.64 KB

For other exceptions in core we just use sprintf(). Simple but effective;)

clemens.tolboom’s picture

This doesn't cover #13

so we need to use SafeMarkup instead.

Should we use this?

sudhanshug’s picture

StatusFileSize
new1.65 KB

Implemented SafeMarkup::format().

Status: Needs review » Needs work

The last submitted patch, 16: include_content_type-2417917-16.patch, failed testing.

sudhanshug’s picture

StatusFileSize
new1.89 KB

Sorry for the previous patch.

sudhanshug’s picture

Status: Needs work » Needs review
zealfire’s picture

Status: Needs review » Needs work

The latest patch only includes the class of SafeMarkup but does not makes use of it. I think we still need to decide whether we should make use of SafeMarkup or make use of patch submitted in #14.

sudhanshug’s picture

Status: Needs work » Needs review
StatusFileSize
new1.9 KB

I think that I uploaded some old patch. Attached is the latest one and implements SafeMarkup::format().

Status: Needs review » Needs work

The last submitted patch, 21: include_content_type-2417917-21.patch, failed testing.

sudhanshug’s picture

StatusFileSize
new1.92 KB

Was able to solve one error but still one more error persists here: Line 99 of core/tests/Drupal/Tests/Core/Routing/ContentTypeHeaderMatcherTest.php
Can somebody help with that?

sudhanshug’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: include_content_type-2417917-22.patch, failed testing.

chx’s picture

➜  d8 git:(migrationplugin) ✗ curl https://www.drupal.org/files/issues/include_content_type-2417917-22.patch|git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1965  100  1965    0     0   3991      0 --:--:-- --:--:-- --:--:--  3993
➜  d8 git:(migrationplugin) ✗ ./vendor/phpunit/phpunit/phpunit -c ./core **/ContentTypeHeaderMatcherTest.php
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

...F

Time: 443 ms, Memory: 6.75Mb

There was 1 failure:

1) Drupal\Tests\Core\Routing\ContentTypeHeaderMatcherTest::testNoRouteFound
Failed asserting that exception message 'No route found that matches the Content-Type header <em class="placeholder">application/hal+json</em>.' contains 'No route found that matches the Content-Type header application/hal+json.'.

FAILURES!
Tests: 4, Assertions: 10, Failures: 1.

The %s gets converted to a placeholder. Perhaps you wanted to use sprintf ? Check for example core/lib/Drupal/Core/DependencyInjection/ClassResolver.php .

chx’s picture

Re #15

➜  d8 git:(migrationplugin) ✗ ag 'throw.*sprintf' core |wc -l
112
➜  d8 git:(migrationplugin) ✗ ag 'throw.*SafeMarkup' core |wc -l
1

We use sprintf.

berdir’s picture

Actually, we use neither these days, just use plain string concatenation.

wim leers’s picture

Status: Needs work » Needs review
Issue tags: +Quickfix
StatusFileSize
new1.82 KB
berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

  • catch committed 6137592 on
    Issue #2417917 by clemens.tolboom, sudhanshug, Wim Leers, joelpittet:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 90a2ed1 on
    Issue #2417917 by clemens.tolboom, sudhanshug, Wim Leers, joelpittet:...

Status: Fixed » Closed (fixed)

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