Support inheriting from any other Eloquent implementation

[antonioribeiro]

If I'm using Ardent, which is derived from Eloquent, how could I also use Revisionable, since PHP doesn't support multiple inheritances.

Would be great if we could set the class from which Revisionable 'inherits' from or is 'created from' internally, so we could use any other implementation of Eloquent with the package.

[duellsy]

Without using a service provider, or changing this into a trait, I'm not sure that's possible in version 1. It might have to be something saved fro version 2 (which was likely going to be trait based).

[thetutlage]

This was a question asked by me on SO, where antonioribeiro was trying to help me, i did a workaround using trait but not sure this is the right way.

It worked for me but i have only tested save method so far. Here's is what i did

Revisionable.php

I changed the class to a trait and it does not extends Eloquent anymore.

trait Revisionable{

Next i made changes to the save method

    public function save(array $rules = array(),array $customMessages = array(),array $options = array(),Closure $beforeSave = null,Closure $afterSave = null)
    {

        $this->beforeSave();
        $saved = parent::save($rules,$customMessages,$options,$beforeSave,$afterSave);
        if ($saved) {
//            $this->afterSave();
        }
        return $saved;
    }

parent::save will now call the ardent save method which requires some parameters to be passed in, the reason i commented out the afterSave() method because the ardent itself calls that method.

It is working fine so far, i saw the history of updates is working fine and also ardent validations are working good so far.

Next i made some changes to my model.

    use LaravelBook\Ardent\Ardent;
    Class Ticket extends Ardent{
        use Venturecraft\Revisionable\Revisionable;
         public static $rules = array(
            'email' => 'required|email',
        );
    }

[duellsy]

It's definitely a valid way of doing it, however we can't put this into the master branch since it will break every site that is currently using the package by extending it.

What you might be able to do (I haven't tried let along tested this) is create your own class MyBaseController which extends Ardent, and uses Revisionable.

Then have your ticket class extend MyBaseController. I'd be intrigued to see how that went as an interim solution.

[edit] scratch that I had a brain fart, since it's currently not a trait, how could MyBaseController possibly use it... sorry.

[thetutlage]

Yes being in the master branch will not make any sense, but i believe this issue could help others using 2 together.

Also 1 problem i faced is re declaration of variables, since it is a trait now i am not able to redefine them.

Example

protected $revisionEnabled = false;

When i turn it off , it gives an error

Ticket and Venturecraft\Revisionable\Revisionable define the same property ($revisionEnabled) in the composition of Ticket. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. Class was composed

So i have to comment out the declaration of the variable in Revisionable class. I will dig more into the code and will see if a concrete solution can be created for the same

[duellsy]

Ahh, OK yeah for sure.

Please do add any notes on things you needed to do to get it working as a trait, as version 2 will most likely be trait based, so any insights would be priceless.

[Antonimo]

Antonioribeiro, we meet again! *(stackoverflow) =)

I also agree that this should be a version 2 / 1.1 in a different branch, using some kind of dependency injection.

Also about

protected $revisionEnabled = false;

make it public or use a getter/setter:

public function disableRevision() {
        $this->storeRevisions = false;
}

[xLink]

Could you not push the implementation to a Trait, and then in the model we are currently extending make it use the new trait? would stop the breaking of backwards compatibility?

We could then slowly begin to change our implementations and everything will still be awesome

[duellsy]

The core problem at the root of all this though, is that a trait cannot extend a class.

Meaning we can't make a trait that extends the current revisionable class, or Eloquent.

Any suggestions?

[xLink]

no but the model you already have (that we already extend), you can make that use the new trait. doing that shouldnt break backwards compatibilty unless you seriously refactor the class.

Everything else can use the new trait and your still winning.

[duellsy]

That will indeed break all sites currently using the package.

Existing implementations will be something like this:

class Page extends Revisionable {

Revisionable then in turn extends Eloquent.

If we were to change revisionable to be a trait, we wouldn't be able to extend Eloquent from revisionable (since traits can't extend classes). Meaning Page wouldn't then be extending Eloquent.

The only way make Page work again would be to change it to:

class Page extends Eloquent {
    use Venturecraft\Revisionable\Revisionable

It wouldn't just be a simple change we can force on existing users... unless I'm not understanding you correctly?

[xLink]

yeah so the trait is the preferable setup for the new users using the package

to keep backwards compatibility with those already using the package all you need to do is add the trait to the model that is already setup

class Revisionable extends Eloquent{
    use Venturecraft\Revisionable\Revisionable

new users use the trait, old users can continue to use the model extension or swap over to the trait

[duellsy]

I think in the end I got what you were talking about, and I'm doing some testing at the moment with that approach.

So far so good, just trying to find a way to allow for the save() method in both the trait, and cases where the original model is extending something like Ardent which also overrides the save() method

[xLink]

haha awesome, this is superbly handy so the upgrade to use the trait would be phenomenal

[duellsy]

All going well, just having some issues in being able to override settings when using as a trait.

User and Venturecraft\Revisionable\RevisionableTrait define the same property ($revisionEnabled) in the composition of User. However, the definition differs and is considered incompatible

[xLink]

add some getters & setters? :D

[duellsy]

Not as simple as that unfortunately, anybody using the existing package will be setting variables directly from their models like this:

class Page extends Revisionable {
    protected $revieionEnabled = false;
}

These are going to fail with the new trait implementations, need to find a fallback for existing users being able to continue to override Revisionable variables the current way.

[duellsy]

I've just pushed quite a big change to the dev branch, which moves the whole structure to being trait based.

It should now work as both a trait, and also as an extendable class as a fallback for existing users.

I've spent quite a long time testing a range of different implementations including:

• Model using it as a simple trait,
• Model using as a trait, and also extending Ardent
• Model extending Revisionable as previous / curreet installations would
• and playing with the extra config items outlined in the readme.

However, to be safe I'd love if some of you would volunteer to do some extra testing with it to make sure no existing usage, or new trait based usage breaks.

[Antonimo]

Note again that traits are in php 5.4

[duellsy]

A very good point.

Perhaps I'll just have to have two versions of Revisionable, one being the existing class, another being the exact same code but defined as a trait.

[duellsy]

OK, new commit.

You now have the option to use the class or the trait. Both are standalone options.

Class tested with php 5.3.27 ✓
Trait tested with php 5.4.19 ✓

[xLink]

Pulling & testing now :)

[xLink]

well thats a pain, currently i got a validation trait thats being used on a few models but cause it is using the boot() method too, its just clashing when trying to add the revisionable class :/

Trait method boot has not been applied, because there are collisions with other trait methods on UserModel

[duellsy]

I haven't tested this so not sure if it'll work, but worth giving this a try:

use \Venturecraft\Revisionable\RevisionableTrait;

class MyBaseModel extends Eloquent 
{

    use RevisionableTrait, YourOtherTrait {
        RevisionableTrait::boot as revisionableBoot;
        YourOtherTrait::boot as yourBoot;
    }

    public function boot()
    {
        yourBoot();
        revisionableBoot();
    }
}

Hopefully, you can add the boot() method to your Model, and have it call the boot() method from each of the traits, which have been renamed to prevent collisions.

[xLink]

okay so had to screw with that idea a bit, but this worked..

    use \Venturecraft\Revisionable\RevisionableTrait,
        \Cysha\Modules\Core\Traits\SelfValidation {

        \Venturecraft\Revisionable\RevisionableTrait::boot as revisionableBoot;
        \Cysha\Modules\Core\Traits\SelfValidation::boot as validationBoot;
    }

    public static function boot() {
        static::validationBoot();
        static::revisionableBoot();
    }

[duellsy]

Ahhh static, awesome, nice work!

Everything working OK with it? Any body else willing to help test?

[xLink]

just playing with it some more actually, seems like its not taking the disableRevisionField() into account correctly..

The Seeder Code..

      $objProduct = new Shop\Product;
      $objProduct->disableRevisionField(array('sku', 'long_description', 'image_path'));
      $objProduct->published = 1;
      $objProduct->attribute_set_id = rand(1, 11);
      $objProduct->name = ucwords($p);
      $objProduct->slug = $objProduct->makeSlug();
      $objProduct->category_id = $objCategory->id;

      $save = $objProduct->save();

      ...

      $faker = \Faker\Factory::create();
      $faker->seed($objProduct->id);

      ...

      $objProduct->sku = $faker->bothify('???###?#');
      $objProduct->long_description = $faker->text(400);
      $save = $objProduct->save();

the result of which is adding 2 revisionable rows for the sku & long_description, please notice its using the same instance of $objProduct

[duellsy]

@xLink hopefully the recent commit will fix this issue, can you grab the latest and give it a try?

[xLink]

yeah not working atall now,

  [ErrorException]
  preg_replace(): Parameter mismatch, pattern is a string while replacement is an array

only does that whilst

$objProduct->disableRevisionField(array('sku', 'long_description', 'image_path'));

is running in the seeder

[duellsy]

@xLink Took me a while to figure out a way to reliable duplicate this in my test app, but latest update should fix.

[xLink]

yeah that looks like it works properly now, took me ages to figure out the problem was actually revisionable too, thought it was something to do with the validation rulesets i had haha

[carcinocron]

Is there any documentation on how to use Revisionable in Traits mode? Is this in early beta?

[duellsy]

The only change is how you setup your model classes, see the dev readme here:

https://github.com/VentureCraft/revisionable/blob/develop/readme.md#the-new-trait-based-implementation

NB This is only in the dev branch right now, until it's deemed stable enough to be merged into the master branch

[carcinocron]

I'll check it out tomorrow, thanks!

[duellsy]

👍