Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Basz feature/accepted model controller plugin #2615

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
6 participants

@weierophinney weierophinney was assigned Oct 2, 2012

Member

Freeaqingme commented Oct 13, 2012

@weierophinney I incorporated @basz 's feedback. Up for review.

Member

Freeaqingme commented Oct 31, 2012

Rebased!

Member

Freeaqingme commented Nov 5, 2012

This plugin allows one to easily and safely define what View Model type should be used based on the accept header string.

<?php
public function foobarAction()
{
   $arr = array(
        'Zend\View\Model\JsonModel' => array(
            'application/json',
            'application/javascript'
        ),
        'Zend\View\Model\FeedModel' => array(
            'application/rss+xml',
            'application/atom+xml'
        ),
        'Zend\View\Model\ViewModel' => '*/*'
    );


    $this->request->getHeaders()->addHeader($header);
    $viewModel = $this->acceptantViewModelSelector($arr);
    //    $viewModel now is an instance of Zend\View\Model\FeedModel
}
?>

Generally this mapping array will be defined on a controller level, or even application wide level. But it does allow one to deviate from the defaults.

The need for this functionality arises because of multiple difficulties currently experienced with accept header handling and view model selection:

  • The default render strategy strategy kicks in only after the controller has dispatched. However, based on the accept header it may be required to perform different actions.
  • The current render strategy setup is not capable of handling accept headers like /.
  • Because render strategies are defined on an application-wide level, security issues arise, eg. zendframework#2410

@akrabat akrabat commented on an outdated diff Nov 5, 2012

.../Mvc/Controller/Plugin/AcceptantViewModelSelector.php
+namespace Zend\Mvc\Controller\Plugin;
+
+use Zend\Http\Request;
+use Zend\Http\Header\Accept\FieldValuePart\AbstractFieldValuePart;
+use Zend\Mvc\Controller\Plugin\AbstractPlugin;
+use Zend\View\Model\ModelInterface;
+use Zend\Mvc\InjectApplicationEventInterface;
+use Zend\Mvc\MvcEvent;
+use Zend\Mvc\Exception\InvalidArgumentException;
+
+/**
+ * @category Zend
+ * @package Zend_Mvc
+ * @subpackage Controller
+ */
+class AcceptantViewModelSelector extends AbstractPlugin
@akrabat

akrabat Nov 5, 2012

Member

I think this should be AcceptedViewModelSelector

Owner

weierophinney commented Nov 5, 2012

My only concern at this point is whether or not the ViewManager should allow registering strategies at initialization or not.

Removing that functionality is technically a BC break, but, as you and others have noted, it could also be a potential security issue.

I'll ask the security team their opinion. Otherwise, this is a great set of functionality, and I look forward to merging it and using it.

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...ader/Accept/FieldValuePart/AbstractFieldValuePart.php
@@ -37,6 +43,28 @@ public function __construct($internalValues)
}
/**
+ * Set a Field Value Part this Field Value Part matched against.
+ *
+ * @param AbstractFieldValuePart $matchedPart
+ * return AbstractFieldValuePart provides fluent interface
@Maks3w

Maks3w Nov 5, 2012

Member

missed @ in front of return

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...ader/Accept/FieldValuePart/AbstractFieldValuePart.php
@@ -37,6 +43,28 @@ public function __construct($internalValues)
}
/**
+ * Set a Field Value Part this Field Value Part matched against.
+ *
+ * @param AbstractFieldValuePart $matchedPart
+ * return AbstractFieldValuePart provides fluent interface
+ */
+ public function setMatchedAgainst(AbstractFieldValuePart $matchedPart)
@Maks3w

Maks3w Nov 5, 2012

Member

IMO setter name should match with field name

@Maks3w Maks3w commented on the diff Nov 5, 2012

library/Zend/Http/Header/AbstractAccept.php
@@ -302,8 +302,10 @@ public function match($matchAgainst)
foreach ($this->getPrioritized() as $left) {
foreach ($matchAgainst as $right) {
if ($right->type == '*' || $left->type == '*') {
- if ($res = $this->matchAcceptParams($left, $right)) {
- return $res;
+ if ($this->matchAcceptParams($left, $right)) {
+ $left->setMatchedAgainst($right);
+
+ return $left;
@Maks3w

Maks3w Nov 5, 2012

Member

This doesn't match with the @return signature, should return bool|array not a AbstractFieldValuePart object.

@Freeaqingme

Freeaqingme Nov 6, 2012

Member

Docblocks were wrong (and had been before). Instead I updated those.

@Maks3w Maks3w commented on the diff Nov 5, 2012

library/Zend/Http/Header/AbstractAccept.php
@@ -313,8 +315,10 @@ public function match($matchAgainst)
($left->format == $right->format ||
$right->format == '*' || $left->format == '*')))
{
- if ($res = $this->matchAcceptParams($left, $right)) {
- return $res;
+ if ($this->matchAcceptParams($left, $right)) {
+ $left->setMatchedAgainst($right);
+
+ return $left;
@Maks3w

Maks3w Nov 5, 2012

Member

same here

@Freeaqingme

Freeaqingme Nov 6, 2012

Member

Same: Docblocks were wrong (and had been before). Instead I updated those.

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...ader/Accept/FieldValuePart/AbstractFieldValuePart.php
@@ -37,6 +43,28 @@ public function __construct($internalValues)
}
/**
+ * Set a Field Value Part this Field Value Part matched against.
+ *
+ * @param AbstractFieldValuePart $matchedPart
+ * return AbstractFieldValuePart provides fluent interface
+ */
+ public function setMatchedAgainst(AbstractFieldValuePart $matchedPart)
+ {
+ $this->matchedPart = $matchedPart;
+ return $this;
+ }
+
+ /**
+ * Get a Field Value Part this Field Value Part matched against.
+ *
+ * return AbstractFieldValuePart|null
@Maks3w

Maks3w Nov 5, 2012

Member

missed @

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ * Default array to match against.
+ *
+ * @var Array
+ */
+ protected $defaultMatchAgainst;
+
+ /**
+ *
+ * @var string Default ViewModel
+ */
+ protected $defaultViewModelName = 'Zend\View\Model\ViewModel';
+
+ /**
+ * Detects an appropriate viewmodel for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
@Maks3w

Maks3w Nov 5, 2012

Member

phpdoc does not accept (optional) at this point

@Param [name] []

@Maks3w

Maks3w Nov 5, 2012

Member

the same in the rest of the block

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ *
+ * @var Array
+ */
+ protected $defaultMatchAgainst;
+
+ /**
+ *
+ * @var string Default ViewModel
+ */
+ protected $defaultViewModelName = 'Zend\View\Model\ViewModel';
+
+ /**
+ * Detects an appropriate viewmodel for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
+ * @param bool (optional)$returnDefault If no match is availble. Return default instead
@Maks3w

Maks3w Nov 5, 2012

Member

s/avilble/available/

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ * @param AbstractFieldValuePart|null (optional) $resultReference The object that was matched
+ * @throws InvalidArgumentException If the supplied and matched View Model could not be found
+ * @return ModelInterface|null
+ */
+ public function __invoke(
+ array $matchAgainst = null,
+ $returnDefault = true,
+ & $resultReference = null)
+ {
+ return $this->getViewModel($matchAgainst, $returnDefault, $resultReference);
+ }
+
+ /**
+ * Detects an appropriate viewmodel for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
@Maks3w

Maks3w Nov 5, 2012

Member

The same observations about phpdoc syntax as above

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ * @throws InvalidArgumentException If the supplied and matched View Model could not be found
+ * @return ModelInterface|null
+ */
+ public function __invoke(
+ array $matchAgainst = null,
+ $returnDefault = true,
+ & $resultReference = null)
+ {
+ return $this->getViewModel($matchAgainst, $returnDefault, $resultReference);
+ }
+
+ /**
+ * Detects an appropriate viewmodel for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
+ * @param bool (optional)$returnDefault If no match is availble. Return default instead
@Maks3w

Maks3w Nov 5, 2012

Member

same typo

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ if (!$name) {
+ return;
+ }
+
+ if (!class_exists($name)) {
+ throw new InvalidArgumentException('The supplied View Model could not be found');
+ }
+
+ return new $name();
+ }
+
+
+ /**
+ * Detects an appropriate viewmodel name for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
@Maks3w

Maks3w Nov 5, 2012

Member

same observations and typos as above

@Maks3w Maks3w and 1 other commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ /**
+ * Detects an appropriate viewmodel name for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
+ * @return AbstractFieldValuePart|null The object that was matched
+ */
+ public function match(array $matchAgainst = null)
+ {
+ $request = $this->getRequest();
+ $headers = $request->getHeaders();
+
+ if ((!$matchAgainst && !$this->defaultMatchString) || !$headers->has('accept')) {
+ return null;
+ }
+
+ if (!$matchAgainst) {
@Maks3w

Maks3w Nov 5, 2012

Member

I suggest move this block before the above check and simplify the complexity of the conditional sentence

@weierophinney

weierophinney Nov 6, 2012

Owner

@Maks3w The previous conditional is more specific, however, and will end execution of the method faster when matched. It makes sense.

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Mvc
+ */
+
+namespace Zend\Mvc\Controller\Plugin;
+
+use Zend\Http\Request;
+use Zend\Http\Header\Accept\FieldValuePart\AbstractFieldValuePart;
+use Zend\Mvc\Controller\Plugin\AbstractPlugin;
+use Zend\View\Model\ModelInterface;
+use Zend\Mvc\InjectApplicationEventInterface;
+use Zend\Mvc\MvcEvent;
+use Zend\Mvc\Exception\InvalidArgumentException;
+
+/**
+ * @category Zend
@Maks3w

Maks3w Nov 5, 2012

Member

If possible add a description about the purpose of the class

@Maks3w Maks3w and 2 others commented on an outdated diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ */
+ protected function getRequest()
+ {
+ if ($this->request) {
+ return $this->request;
+ }
+
+ $event = $this->getEvent();
+ $request = $event->getRequest();
+ if (!$request instanceof Request) {
+ throw new Exception\DomainException(
+ 'The event used does not contain a valid Request, but must.'
+ );
+ }
+
+ return $this->request = $request;
@Maks3w

Maks3w Nov 5, 2012

Member

This is not very usual.

@Freeaqingme

Freeaqingme Nov 6, 2012

Member

@Maks3w What do you propose?

@Maks3w

Maks3w Nov 6, 2012

Member

The same as the lines 236 & 238.

To be clear. I don't say if it's a bad practice or not, there is no rules about this. My concerns are about to make code with a predictable style and widely supported along the time. What do I mean? I mean even being a valid syntax, the could have the possibility of not work with future releases of PHP, simply because is a weird syntax. Something similar like the other day where a contributor proposed !!$var instead (bool) $var

@weierophinney

weierophinney Nov 6, 2012

Owner

@Freeaqingme What @Maks3w is proposing is you do the assignment and then return:

$this->request = $request;
return $request;

@Maks3w Maks3w commented on an outdated diff Nov 5, 2012

...Controller/Plugin/AcceptableViewModelSelectorTest.php
+ $this->request->getHeaders()->addHeader($header);
+
+ $result = $plugin->match(array( 'Zend\View\Model\ViewModel' => '*/*'));
+ $this->assertInstanceOf(
+ 'Zend\Http\Header\Accept\FieldValuePart\AcceptFieldValuePart',
+ $result
+ );
+ }
+
+ public function testInvalidModel()
+ {
+ $arr = array('DoesNotExist' => 'text/xml');
+ $header = Accept::fromString('Accept: */*');
+ $this->request->getHeaders()->addHeader($header);
+
+ try {
@Maks3w

Maks3w Nov 5, 2012

Member

use $this->setExpectedException instead of try/catch block

@Maks3w Maks3w and 2 others commented on an outdated diff Nov 5, 2012

...Controller/Plugin/AcceptableViewModelSelectorTest.php
+ 'application/json',
+ 'application/javascript'
+ ),
+ 'Zend\View\Model\FeedModel' => array(
+ 'application/rss+xml',
+ 'application/atom+xml'
+ ),
+ 'Zend\View\Model\ViewModel' => '*/*'
+ );
+
+ $plugin = $this->plugin;
+ $header = Accept::fromString('Accept: text/plain; q=0.5, text/html, text/xml; q=0, text/x-dvi; q=0.8, text/x-c');
+ $this->request->getHeaders()->addHeader($header);
+ $result = $plugin($arr);
+
+ $this->assertInstanceOf('Zend\View\Model\ViewModel', $result);
@Maks3w

Maks3w Nov 5, 2012

Member

It's not clear if return ViewModel due */* or because is the default value of defaultViewModelName

@weierophinney

weierophinney Nov 6, 2012

Owner

Agreed with @Maks3w -- @Freeaqingme -- I think you need to add some assertNotInstanceOf() clauses here to ensure you don't have a Json or Feed model, as those extend ViewModel, and it's clear you're expecting specifically ViewModel, not an extension.

@Freeaqingme

Freeaqingme Nov 6, 2012

Member

Although yuu both mention different (yet equally important) points, I fixed them both.

@Maks3w Maks3w commented on the diff Nov 5, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ * @var Zend\Mvc\MvcEvent
+ */
+ protected $event;
+
+ /**
+ *
+ * @var Zend\Http\Request
+ */
+ protected $request;
+
+ /**
+ * Default array to match against.
+ *
+ * @var Array
+ */
+ protected $defaultMatchAgainst;
@Maks3w

Maks3w Nov 5, 2012

Member

What method set a value to this field?

@Freeaqingme

Freeaqingme Nov 6, 2012

Member

As discussed, a getter and setter were added.

@weierophinney weierophinney and 1 other commented on an outdated diff Nov 6, 2012

...Mvc/Controller/Plugin/AcceptableViewModelSelector.php
+ protected $defaultViewModelName = 'Zend\View\Model\ViewModel';
+
+ /**
+ * Detects an appropriate viewmodel for request.
+ *
+ * @param array (optional) $matchAgainst The Array to match against
+ * @param bool (optional)$returnDefault If no match is availble. Return default instead
+ * @param AbstractFieldValuePart|null (optional) $resultReference The object that was matched
+ * @throws InvalidArgumentException If the supplied and matched View Model could not be found
+ * @return ModelInterface|null
+ */
+ public function __invoke(
+ array $matchAgainst = null,
+ $returnDefault = true,
+ & $resultReference = null)
+ {
@weierophinney

weierophinney Nov 6, 2012

Owner

CS dictates moving the closing paren to the same line as the opening brace in these situations, and only one level of indentation for the arguments:

public function __invoke(
    array $matchAgainst = null,
    $returnDefault = true,
    & $resultReference = null
) {
@Freeaqingme

Freeaqingme Nov 6, 2012

Member

Fixed. Also in the rest of the class.

Contributor

juriansluiman commented Nov 7, 2012

Would it be possible to make some use cases a bit easier to work with? In my particular case, I have quite often support for a Json model when application/json or application/javascript is requested, all other types are simply handled the same way. In case JSON is requested, it will always be for q=1 (so at top-priority).

I could imagine a simple way like this:

class MyController
{
  public function fooAction()
  {
    $service = $this->getService();
    $result  = $service->doSomeWork();

    if ($this->accept()->match(array('application/json', 'application/javascript'))) {
      return new JsonModel($result);
    }

    return $this->redirect()->toRoute('foo/bar');
  }
}

I can create my own plugin using Request::match() myself, but it seems something like this could also be implemented in this AcceptableViewModelSelector?

Member

Maks3w commented Nov 7, 2012

@basz @Freeaqingme Don't forget send a PR to zf2-documentation documenting the plugn!

Contributor

basz commented Nov 7, 2012

I can start with this... but not within a few days...

Member

Freeaqingme commented Nov 7, 2012

@juriansluiman We could indeed implement such functionality. However, it should be 100% configurable, and may need a little bit more thought put into it. Lets get this baby done first, then look what else we could do.

@Maks3w No worries, you're getting your docs! ;)

Owner

weierophinney commented Nov 7, 2012

@Freeaqingme As noted yesterday on IRC, we'll need to merge this against the master branch as well. The only way I see to do this is to merge a range of commits or cherry-pick. My question is: do you want to do this, or are you okay with somebody else doing it? Both will rewrite history, so the question is if you want that rewrite to happen in your own branch, or in mine.

@weierophinney weierophinney added a commit to weierophinney/zendframework that referenced this pull request Nov 7, 2012

@weierophinney weierophinney [#2410][#2615] Update readme
- Added security note about change to JsonStrategy and FeedStrategy, and
  also detail AcceptableViewModelSelector usage
b8b2ddc

@weierophinney weierophinney added a commit to weierophinney/zendframework that referenced this pull request Nov 7, 2012

@weierophinney weierophinney [#2410][#2615] s/model/viewModel/
- per @Akrabat
07f20f8

@weierophinney weierophinney added a commit to weierophinney/zendframework that referenced this pull request Nov 7, 2012

@weierophinney weierophinney [#2615] Test shuffling
- Split testInvokeWithoutDefaults() into two separate tests, for the
  separate behaviors it tested
  (testInvokeWithoutDefaultsReturnsNullWhenNoMatchesOccur() and
  testInvokeReturnsFieldValuePartOnMatchWhenReferenceProvided())
- Renamed testInvoke__2() to
  testHonorsAcceptPrecedenceAndPriorityWhenInvoked()
b1f0fbb

@weierophinney weierophinney added a commit that referenced this pull request Nov 7, 2012

@weierophinney weierophinney Merge branch 'hotfix/2615' into develop
Forward port #2615 and #2410

Conflicts:
	library/Zend/Mvc/Controller/PluginManager.php
8dae483

@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/2615'
Introduce AcceptableViewModelSelector, and remove Accept matching from
JsonStrategy and FeedStrategy

Close zendframework/zendframework#2615
Close zendframework/zendframework#2410
1e41fbd

@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/2615' into develop
Forward port zendframework/zendframework#2615 and zendframework/zendframework#2410

Conflicts:
	library/Zend/Mvc/Controller/PluginManager.php
ec1f916

@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/2615'
Introduce AcceptableViewModelSelector, and remove Accept matching from
JsonStrategy and FeedStrategy

Close zendframework/zendframework#2615
Close zendframework/zendframework#2410
0ebadc5

@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/2615' into develop
Forward port zendframework/zendframework#2615 and zendframework/zendframework#2410

Conflicts:
	library/Zend/Mvc/Controller/PluginManager.php
8ba0094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment