Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controller events #153

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 0 additions & 53 deletions src/ZfrRest/Mvc/Controller/AbstractRestfulController.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

use Doctrine\Common\Collections\Collection;
use Zend\Http\Request as HttpRequest;
use Zend\InputFilter\InputFilterInterface;
use Zend\InputFilter\InputFilterPluginManager;
use Zend\Mvc\Controller\AbstractController;
use Zend\Mvc\MvcEvent;
use Zend\Paginator\Paginator;
Expand All @@ -48,25 +46,6 @@ class AbstractRestfulController extends AbstractController
*/
protected $methodHandlerManager;

/**
* If this is set to true, then controller will automatically instantiate the input filter specified in
* resource metadata (if there is one) - from service locator first, or directly instantiate it if not found -,
* and validate data. If data is incorrect, it will return a 400 HTTP error (Bad Request) with the failed
* validation messages in it).
*
* @var bool
*/
protected $autoValidate = true;

/**
* If this is set to true, then controller will automatically instantiate the hydrator specified in resource
* metadata (if there is one) - from service locator first, or directly instantiate it if not found - and
* hydrate resource object with previously validated data
*
* @var bool
*/
protected $autoHydrate = true;

/**
* {@inheritDoc}
*/
Expand Down Expand Up @@ -133,36 +112,4 @@ public function getMethodHandlerManager()

return $this->methodHandlerManager;
}

/**
* Hook to configure an input filter fetched/created by ZfrRest
*
* @param InputFilterPluginManager $inputFilterPluginManager
* @param string $inputFilterName
* @return InputFilterInterface
*/
public function getInputFilter(InputFilterPluginManager $inputFilterPluginManager, $inputFilterName)
{
return $inputFilterPluginManager->get($inputFilterName);
}

/**
* Should auto validate?
*
* @return bool
*/
public function getAutoValidate()
{
return $this->autoValidate;
}

/**
* Should auto hydrate?
*
* @return bool
*/
public function getAutoHydrate()
{
return $this->autoHydrate;
}
}
115 changes: 115 additions & 0 deletions src/ZfrRest/Mvc/Controller/Event/HydrationEvent.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license.
*/

namespace ZfrRest\Mvc\Controller\Event;

use Zend\EventManager\Event;
use Zend\Stdlib\Hydrator\HydratorInterface;
use Zend\Stdlib\Hydrator\HydratorPluginManager;
use ZfrRest\Resource\ResourceInterface;

/**
* @author Daniel Gimenes <daniel@danielgimenes.com.br>
* @licence MIT
*/
class HydrationEvent extends Event
{
/**
* Event names
*/
const EVENT_HYDRATE_PRE = 'hydrate.pre';
const EVENT_HYDRATE_POST = 'hydrate.post';

/**
* @var bool
*/
protected $autoHydrate = true;

/**
* @var ResourceInterface
*/
protected $resource;

/**
* @var HydratorPluginManager
*/
protected $hydratorManager;

/**
* @var null|HydratorInterface
*/
protected $hydrator;

/**
* @param ResourceInterface $resource
* @param HydratorPluginManager $hydratorManager
*/
public function __construct(ResourceInterface $resource, HydratorPluginManager $hydratorManager)
{
$this->resource = $resource;
$this->hydratorManager = $hydratorManager;
}

/**
* @param bool $autoHydrate
*/
public function setAutoHydrate($autoHydrate)
{
$this->autoHydrate = (bool) $autoHydrate;
}

/**
* @return bool
*/
public function getAutoHydrate()
{
return $this->autoHydrate;
}

/**
* @return ResourceInterface
*/
public function getResource()
{
return $this->resource;
}

/**
* @return HydratorPluginManager
*/
public function getHydratorManager()
{
return $this->hydratorManager;
}

/**
* @param HydratorInterface $hydrator
*/
public function setHydrator(HydratorInterface $hydrator)
{
$this->hydrator = $hydrator;
}

/**
* @return null|HydratorInterface
*/
public function getHydrator()
{
return $this->hydrator;
}
}
116 changes: 116 additions & 0 deletions src/ZfrRest/Mvc/Controller/Event/ValidationEvent.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license.
*/

namespace ZfrRest\Mvc\Controller\Event;

use Zend\EventManager\Event;
use Zend\InputFilter\InputFilterInterface;
use Zend\InputFilter\InputFilterPluginManager;
use ZfrRest\Resource\ResourceInterface;

/**
* @author Daniel Gimenes <daniel@danielgimenes.com.br>
* @licence MIT
*/
class ValidationEvent extends Event
{
/**
* Event names
*/
const EVENT_VALIDATE_PRE = 'validate.pre';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably have a mirrored validate.post

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I told, but I think there is nearly no usage to having this event. But we may add it for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should I replace validate.error and validate.success by validate.post?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Post could be an additional event triggered in both cases. But again, I'm really not sure about the usefulness of it... I'd say that we should not include it, and if someone really need that... we'll include it.

const EVENT_VALIDATE_ERROR = 'validate.error';
const EVENT_VALIDATE_SUCCESS = 'validate.success';

/**
* @var bool
*/
protected $autoValidate = true;

/**
* @var ResourceInterface
*/
protected $resource;

/**
* @var InputFilterPluginManager
*/
protected $inputFilterManager;

/**
* @var null|InputFilterInterface
*/
protected $inputFilter;

/**
* @param ResourceInterface $resource
* @param InputFilterPluginManager $inputFilterManager
*/
public function __construct(ResourceInterface $resource, InputFilterPluginManager $inputFilterManager)
{
$this->resource = $resource;
$this->inputFilterManager = $inputFilterManager;
}

/**
* @param bool $autoValidate
*/
public function setAutoValidate($autoValidate)
{
$this->autoValidate = (bool) $autoValidate;
}

/**
* @return bool
*/
public function getAutoValidate()
{
return $this->autoValidate;
}

/**
* @return ResourceInterface
*/
public function getResource()
{
return $this->resource;
}

/**
* @return InputFilterPluginManager
*/
public function getInputFilterManager()
{
return $this->inputFilterManager;
}

/**
* @param InputFilterInterface $inputFilter
*/
public function setInputFilter(InputFilterInterface $inputFilter)
{
$this->inputFilter = $inputFilter;
}

/**
* @return null|InputFilterInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a hard dependency, cannot be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it will be null until you set it :)

*/
public function getInputFilter()
{
return $this->inputFilter;
}
}
37 changes: 31 additions & 6 deletions src/ZfrRest/Mvc/Controller/MethodHandler/DataHydrationTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@

namespace ZfrRest\Mvc\Controller\MethodHandler;

use Zend\EventManager\EventManagerAwareInterface;
use Zend\Stdlib\Hydrator\HydratorInterface;
use Zend\Stdlib\Hydrator\HydratorPluginManager;
use ZfrRest\Mvc\Controller\Event\HydrationEvent;
use ZfrRest\Mvc\Exception\RuntimeException;
use ZfrRest\Resource\ResourceInterface;

Expand All @@ -38,18 +41,40 @@ trait DataHydrationTrait
/**
* Hydrate the object bound to the data
*
* @param ResourceInterface $resource
* @param array $data
* @param ResourceInterface $resource
* @param array $data
* @param EventManagerAwareInterface $controller
* @return array|object
* @throws RuntimeException
*/
public function hydrateData(ResourceInterface $resource, array $data)
public function hydrateData(ResourceInterface $resource, array $data, EventManagerAwareInterface $controller)
{
if (!($hydratorName = $resource->getMetadata()->getHydratorName())) {
throw new RuntimeException('No hydrator name has been found in resource metadata');
/* @var EventManagerInterface $eventManager */
$eventManager = $controller->getEventManager();

$event = new HydrationEvent($resource, $this->hydratorPluginManager);
$event->setTarget($controller);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the target a constructor param?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep


$eventManager->trigger(HydrationEvent::EVENT_HYDRATE_PRE, $event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the name in the event itself (constructor)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this way personally. It's more explicit as you can directly see the event name in trigger. Having it in the event is confusing, you have to locate the event... Especially as this method triggers two different items, it's much much clearer this way. Please leave it like this @danizord

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok


if (!$event->getAutoHydrate()) {
return $data;
}

/* @var HydratorInterface $inputFilter */
$hydrator = $event->getHydrator();

if (!$hydrator instanceof HydratorInterface) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 67~74 should be handled IN the event listeners, not in this trait. Instead, pass the hydrator name to the constructor of the event, then let the listener do whatever it wants with this data.

It actually only needs the metadata, not even the precise hydrator name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically what you want is moving this logic to the "setHydrator" method of the event? @danizord what do you think? (the same would apply to input filter I suppose)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's reasonable IMHO, but this way we will need to provide a default listener to the event. So, If we go this path, I'd say we should move the complete hydration proccess to the listener, so the trait will trigger hydrate event (with $data and $resource) and expect some listener to return the hydrated object. That's a lot flexible and decoupled (:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made two RP to your branch @danizord that implement that ;).

if (!($hydratorName = $resource->getMetadata()->getHydratorName())) {
throw new RuntimeException('No hydrator name has been found in resource metadata');
}

$hydrator = $this->hydratorPluginManager->get($hydratorName);

$event->setHydrator($hydrator);
}

$hydrator = $this->hydratorPluginManager->get($hydratorName);
$eventManager->trigger(HydrationEvent::EVENT_HYDRATE_POST, $event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of use case could we have from this event? I'd like not to introduce useless events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if there's a use case for that event (maybe some cache stuff?). But the point is that we have a hydrate.pre, so we should have hydrate.post just for the sake of it :D

We can remove it if you want, but we first need a better name for hydration event (not .pre).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep both then :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If lines 67~74 are removed, then there is no need for .pre and .post events, we can just use the event manager priorities and save some cpu cycles here.


return $hydrator->hydrate($data, $resource->getData());
}
Expand Down
Loading