Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

AcceptableViewModelSelector returns wrong View Model #194

Closed
MatthiasKuehneEllerhold opened this issue Aug 16, 2016 · 4 comments
Closed

Comments

@MatthiasKuehneEllerhold
Copy link
Contributor

I tried using the AcceptableViewModelSelector with this Accept-Header in my request (Its the standard header for HTML in FF):
text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

Expectation:

It should return "ViewModel" with the given Accept-Header and it should return a JsonModel with "application/json" in my Accept-Header.

Reality:

Testcase 1:

$viewModel = $this->acceptableViewModelSelector(
    [
        JsonModel::class => ['application/json'],
        FeedModel::class => ['application/rss+xml'],
    ]
);

Returns

  • JsonModel if application/json
  • FeedModel if application/rss+xml
  • JsonModel with the Accept-Header above

Testcase 2:

$viewModel = $this->acceptableViewModelSelector(
    [
        FeedModel::class => ['application/rss+xml'],
        JsonModel::class => ['application/json'],
    ]
);

Returns

  • JsonModel if application/json
  • FeedModel if application/rss+xml
  • FeedModel with the Accept-Header above

Testcase 3:

$viewModel = $this->acceptableViewModelSelector(
    [
        FeedModel::class => ['application/rss+xml'],
        JsonModel::class => ['application/json'],
        ViewModel::class => ['text/html'],
    ]
);

Returns

  • JsonModel if application/json
  • FeedModel if application/rss+xml
  • ViewModel with the Accept-Header above

Observation

It seems like the fallback to defaultViewModel does not work and it'll returns the first Model it gets if no match was successfull.

@MatthiasKuehneEllerhold
Copy link
Contributor Author

MatthiasKuehneEllerhold commented Aug 16, 2016

PS:
I tried it with

  • PHP 7.0.9-1~dotdeb+8.1
  • zendframework/zend-mvc 3.0.2
  • zendframework/zend-view 2.8.1
  • zendframework/zend-http 2.5.5

Possibly related to #16 ?

@adamlundrigan
Copy link
Contributor

I added this test to the zend-mvc test suite:

    public function testAppropriateFallbackOrderTestCase1()
    {
        $arr = [
            'Zend\View\Model\JsonModel' => ['application/json'],
            'Zend\View\Model\FeedModel' => ['application/rss+xml'],
        ];

        $header   = Accept::fromString('text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8');
        $this->request->getHeaders()->addHeader($header);
        $plugin   = $this->plugin;
        $result   = $plugin($arr);

        $this->assertInstanceOf('Zend\View\Model\ViewModel', $result);
        $this->assertNotInstanceOf('Zend\View\Model\FeedModel', $result); 
        $this->assertNotInstanceOf('Zend\View\Model\JsonModel', $result);
    }

and it does fail because $result is a JsonModel, as you suggested it would. After reviewing the code I think it's a subtle bug in the way AcceptableViewModelSelector builds the accept string used to match against the Accept header. The default model is not included in the match string when it's constructed (here), so AcceptableViewModelSelector sends this to the Accept header for matching:

application/json; _internalViewModel="Zend|View|Model|JsonModel", application/rss+xml; _internalViewModel="Zend|View|Model|FeedModel", 

As those match none of the explicit types specified in the Accept header, it comes down to that last piece */*;q=0.8 which matches everything, and the first acceptable model in the list will always match that.

So, this is caused by AcceptableViewModelSelector not adding the default view model with content type text/html to the list of acceptable types matched by the Accept header. If the match string looked like this instead:

application/json; _internalViewModel="Zend|View|Model|JsonModel", 
application/rss+xml; _internalViewModel="Zend|View|Model|FeedModel", 
text/html; _internalViewModel="Zend|View|Model|ViewModel",

It would work (as your Test Case 3 above shows).

TL;DR: when specifying a list of acceptable view models, specify every acceptable one and don't rely on the fallback to render the HTML view. It can't be added automatically to the list as there might be cases where HTML isn't an acceptable return and you'd then have to opt-out of that by setting a different default.

My recommendation is to update the docs for AcceptableViewModelSelector to make it clear that when specifying a list of acceptable models you must specify every acceptable type.

@MatthiasKuehneEllerhold
Copy link
Contributor Author

MatthiasKuehneEllerhold commented Aug 16, 2016

In reality every browser appends */* at the end of the Accept Header so the default view model is only used for the non-browser requests?

  • FF AJAX: application/json, text/javascript, */*; q=0.01
  • Chrome HTML: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
  • Chrome AJAX: application/json, text/javascript, */*; q=0.01

@adamlundrigan
Copy link
Contributor

Essentially, yes. The default is used in only two cases:

  1. An empty list of acceptable views models was given.
  2. There are no matches against the Accept header.

However 2 is moot if the Accept header has a *.* since it will always match something.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants