Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

validating and validated events observable fix #153

Merged
merged 1 commit into from
Feb 25, 2014
Merged

validating and validated events observable fix #153

merged 1 commit into from
Feb 25, 2014

Conversation

unstoppablecarl
Copy link
Contributor

The validating event works when bound in the boot() function with the following code.

<?php
class Record extends Ardent{
    static public function boot(){
        parent::boot();
        self::validating(function($record){
            // this always works
        });
    }
}

The observable object binding of the validating event only works if the $observables property includes "validating" in its array and is subsequently returned in the array when calling getObservableEvents().

<?php
class Record extends Ardent{
    // only works if this is set
    public $observables = array('validating');
}
class Observer {
    public function validating($model){
        // works if $observables = array('validating');
    }
}

Record::observe(new Observer);

The getObservableEvents() needs to be extended in the Ardent class to work correctly.

The validating event works when bound in the boot() function with the following code.

<?php
class Record extends Ardent{
    static public function boot(){
        parent::boot();
        self::validating(function($record){
            // this always works
        });
    }
}

The observable object binding of the validating event only works if the $observables property includes "validating" in its array and is subsequently returned in the array when calling getObservableEvents().

<?php
class Record extends Ardent{
    // only works if this is set
    public $observables = array('validating');
}
class Observer {
    public function validating($model){
        // works if $observables = array('validating');
    }
}

Record::observe(new Observer);

The getObservableEvents() needs to be extended in the Ardent class to work correctly.
@igorsantos07
Copy link
Member

Sorry if I missed something, as I'm in a horrible connection and I can't search codebases now. Where did this Observer class came from? Is it present in Ardent's or Laravel's code?

If yes, please show me it.
If no and this is something yours, please implement it as your own fork or, even simpler, extend Ardent in the project that needs Observers and implement it there (:

@unstoppablecarl
Copy link
Contributor Author

The eloquent ORM Model class uses the following events: creating, created, updating, updated, saving, saved, deleting, deleted, restoring, restored. see http://laravel.com/docs/eloquent#model-events

Ardent adds "validating" and "validated" events.

Some of the relevant code in the Eloquent Model class can be seen here https://github.com/illuminate/database/blob/master/Eloquent/Model.php#L1020

In ardent it is duplicated here https://github.com/laravelbook/ardent/blob/master/src/LaravelBook/Ardent/Ardent.php#L241

This implementation is enough for the events to work consistently with the Eloquent Model class in my first example. My second example does not work because the "validating" and "validated" events are not returned in the getObservableEvents() function https://github.com/illuminate/database/blob/master/Eloquent/Model.php#L1163

I believe the intent was to add the "validating" and "validated" events and have them function consistently with all the Eloquent model events but the way the "validating" and "validated" events are implemented in Ardent is inconsistent with the Eloquent Model events as it is not returned in the getObservableEvents() function.

@igorsantos07
Copy link
Member

Oh! I got it now!
I never knew that Eloquent supported Observables.

Just a comment: if we're extending the class we should extend instead of override the method, right? (:
I'll commit a better proposal for this idea soon (:

@unstoppablecarl
Copy link
Contributor Author

I wasn't sure how to best implement it as it merged the arrays. After looking at it a second time I realize I was over thinking it. It would probably be best to just do.

public function getObservableEvents(){
    return array_merge(
        parent::getObservableEvents(),
        array('validating', 'validated')
    );
}

igorsantos07 added a commit that referenced this pull request Feb 25, 2014
Including observable events for 'validating' and 'validated'
@igorsantos07 igorsantos07 merged commit 2d26521 into laravel-ardent:master Feb 25, 2014
@igorsantos07
Copy link
Member

I've merged and implemented the better version on 0a0238b (:

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

Successfully merging this pull request may close these issues.

2 participants