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

Added events. #9

Closed
wants to merge 1 commit into from
Closed

Added events. #9

wants to merge 1 commit into from

Conversation

bweston92
Copy link

I would of done this in the performValidation method but doesn't always
get called (Having looked at saving method)

Passing the model allows the event handler to know what class it is and
what is dirty on the model, no other information I think is necessary.

I was going to just add a constructor and assign
Model::getEventDispatcher() locally to the instance but the Facade is
fine and testable.

I would of done this in the performValidation method but doesn't always
get called (Having looked at saving method)

Passing the model allows the event handler to know what class it is and
what is dirty on the model, no other information I think is necessary.

I was going to just add a constructor and assign
Model::getEventDispatcher() locally to the instance but the Facade is
fine and testable.
@dwightwatson
Copy link
Owner

Thanks for doing this. A couple of things;

  • I think for consistency we should have 'validating' and 'validated' events. I also don't think that the event that fired the validation ('creating', 'saving', etc.) needs to be appended to the event name.
  • Does it matter that that the event may not get fired on the 'saving' event if there aren't rulesets? I'm not sure, curious as to what you think.
  • You can just return the event rather than comparing it to a Boolean and then returning a Boolean, no?

I apologise for the delay in responding, am currently travelling. Really appreciate the PR.

@bweston92
Copy link
Author

I agree on the naming consistency. The event can return false is the handler does, which also means the saving method does need to call it. I will have to double check now I think of it the method is "until" instead if "fire" will check again.—
Sent from Mailbox for iPhone

On Wed, Jun 11, 2014 at 5:49 AM, Dwight Watson notifications@github.com
wrote:

Thanks for doing this. A couple of things;

  • I think for consistency we should have 'validating' and 'validated' events. I also don't think that the event that fired the validation ('creating', 'saving', etc.) needs to be appended to the event name.
  • Does it matter that that the event may not get fired on the 'saving' event if there aren't rulesets? I'm not sure, curious as to what you think.
  • You can just return the event rather than comparing it to a Boolean and then returning a Boolean, no?
    I apologise for the delay in responding, am currently travelling. Really appreciate the PR.

    Reply to this email directly or view it on GitHub:
    Added events. #9 (comment)

@bweston92
Copy link
Author

Ok, I want your input on this if possible. There is 2 ways this can be done, we can use file and specify the $halt parameter and get all the responses. Or we can use until and that returns the first non-null value returned to us.

Using until I believe would be best, and therefore callbacks would only return false if not to proceed, would need to explicitly say do not return any other value.

@dwightwatson
Copy link
Owner

I'm going to have a good read up on events in Laravel and get back to you on this one. At a glance until() sounds like the go but I am not overly familiar with either yet.

@dwightwatson
Copy link
Owner

until() looks like the best bet. I'm not sure why anyone would want to hook into a validating event and then cancel it but it's handy to have. If you could therefore implement validating and validated events here it should be good to merge. Please also add illuminate/events to the composer.json file as it will be an extra dependency.

@bweston92
Copy link
Author

Ok the facade exists anyway in the support package but I will add it.

Are you wanting the validated to get called directly after the validating event? Or after the performValidation method?

@dwightwatson
Copy link
Owner

I suppose it should be called after performValidation as that is when validation will have completed.

Also, would you be able to target this to the develop branch? Going to put this into the 0.8.x release with a few other substantial changes.

On 12 Jun 2014, at 7:37 am, Bradley Weston notifications@github.com wrote:

Ok the facade exists anyway in the support package but I will add it.

Are you wanting the validated to get called directly after the validating event? Or after the performValidation method?


Reply to this email directly or view it on GitHub.

@dwightwatson dwightwatson added this to the 0.8.x milestone Jun 14, 2014
@dwightwatson dwightwatson modified the milestones: 0.8.1, 0.8.x Jun 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants