[3.x/4.x]: craft\web\Request::accepts() doesn't deal with wildcards

[msbit]

What happened?

When submitting a request with the header:

Accept: */*

craft\web\Request::accepts('application/json') will return false as it's explicitly checking for the existence of application/json.

From my understanding of the header, it should return true, as it should for application/* (and */*, trivially 😀).

This is the case both on Craft 3 and Craft 4.

Reproduction:

$request = new \craft\web\Request();
$request->setAcceptableContentTypes($request->parseAcceptHeader('*/*'));
var_dump($request->accepts('application/json'));
var_dump($request->accepts('application/*'));
var_dump($request->accepts('*/*'));

outputs:

bool(false)
bool(false)
bool(true)

Craft CMS version

3.8.9

PHP version

No response

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

No response

[brandonkelly]

This is resolved for the next Craft 4 release, via #13802.

Supporting */* seems a little dangerous, as we currently check accepts('application/json') when determining whether we should send JSON responses back, and I’m worried we’d start getting some false positives with */*.

[msbit]

@brandonkelly yep, that check (Request::getAcceptsJson) is what brought me to this issue.

I’m worried we’d start getting some false positives with /

Unfortunately I'd say that's likely, though it depends on how "false" you'd rate those positives; if a user agent specifically says they will Accept anything (*/*) I'd say it's on them.

Perhaps the call-sites need work then, or a change in the stated intent (getAcceptsJson -> getAcceptsStrictlyJson ?) along with getAcceptsImage (to getAcceptsAnyImage ?).

[brandonkelly]

Craft 4.5.7 is out with that change.

Perhaps the call-sites need work then, or a change in the stated intent (getAcceptsJson -> getAcceptsStrictlyJson ?) along with getAcceptsImage (to getAcceptsAnyImage ?).

Maybe that would be more semantic, but would be a pretty big breaking change too, and getAcceptsJson() was originally added as a more semantic/reliable replacement for getIsAjax() (which relies on a nonstandard X-Requested-With: XMLHttpRequest header).

I’d be more inclined to add a new getAcceptsAny() method you could call in addition to getAcceptsJson() etc. Feel free to post a new discussion for that, if you feel there’s a need for it.