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

Conversation

danizord
Copy link
Contributor

This is a try to add more flexibility to validation and hydration behaviors of ZfrRest. Not sure if I got the best approach yet tbh, so I opened this PR as a request for comments.

@bakura10 bakura10 added this to the 0.3.0 milestone Apr 23, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+2.11%) when pulling ae1e65e on danizord:feature/controller-events into e62c997 on zf-fr:master.

@@ -49,25 +47,6 @@ class AbstractRestfulController extends AbstractController
protected $methodHandlerManager;

/**
* If this is set to true, then controller will automatically instantiate the input filter specified in
Copy link
Member

Choose a reason for hiding this comment

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

I agree for the controller. This is definitely the way to go.

@bakura10
Copy link
Member

I really like it. It removes a lot of crap from the controller, make everything more flexible... that's really nice.

@bakura10
Copy link
Member

Also, please update doc :D.

@bakura10
Copy link
Member

Just a rough idea, maybe it's plain stupid, but for easier usage, may we automatically attach listeners in the controller (like "preInputFilter", "postInputFilter"...), with empty functions defined by default. So if someone want to have specific behaviour, they can just override the method, and do not need the hassle of manually attaching the listeners.

@bakura10
Copy link
Member

bakura10 commented May 1, 2014

Any news? :d

@danizord
Copy link
Contributor Author

danizord commented May 2, 2014

@bakura10 trying to rebase it

@coveralls
Copy link

Coverage Status

Coverage increased (+1.76%) when pulling 553dc70 on danizord:feature/controller-events into 3ea4570 on zf-fr:master.

@danizord danizord changed the title [WIP] Controller events Controller events May 2, 2014
@danizord
Copy link
Contributor Author

danizord commented May 2, 2014

Ping @bakura10 @Ocramius

Ready for review (:

@coveralls
Copy link

Coverage Status

Coverage increased (+1.76%) when pulling fa51553 on danizord:feature/controller-events into 3ea4570 on zf-fr:master.

@danizord
Copy link
Contributor Author

danizord commented May 2, 2014

Just a rough idea, maybe it's plain stupid, but for easier usage, may we automatically attach listeners in the controller (like "preInputFilter", "postInputFilter"...), with empty functions defined by default. So if someone want to have specific behaviour, they can just override the method, and do not need the hassle of manually attaching the listeners.

Nice idea, I'll do it. (Not sure if post* is that useful, though)

@bakura10
Copy link
Member

bakura10 commented May 2, 2014

Looks very good to me. Let's just wait for an @Ocramius review :).

/**
* 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.

@bakura10
Copy link
Member

Any news about your new idea? :D

@bakura10 bakura10 modified the milestones: 0.4.0, 0.3.0 May 13, 2014
@bakura10
Copy link
Member

Too bad this was never finished :(.

@bakura10
Copy link
Member

I'm closing, #184 will bring a whole new architecture that is not backward-compatible.

@bakura10 bakura10 closed this Jan 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants