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

Feature Request #56

Closed
pine3ree opened this issue Apr 26, 2015 · 17 comments
Closed

Feature Request #56

pine3ree opened this issue Apr 26, 2015 · 17 comments

Comments

@pine3ree
Copy link

@TomBZombie
Hi Tom,
it would be nice to reduce the number of lines of code when implementing a rule.
A couple of simple ways to achieve that:

//1. simply allowing rule definitions with arrays
$dice->addRule('PDO', [
    'shared' => true,
    'constructParams' => ['mysql:host=127.0.0.1;dbname=mydb', 'username', 'password'],
]);
//2. adding setters into the Rule class to obtain a fluent interface
$dice->addRule('PDO', (new Rule())->setShared(true)->setContructParams([]));
@garrettw
Copy link

Good ideas. What I did in my fork was modify the Rule constructor to accept an array of settings, similar to your example 1. So I can do:
$dice->addRule('PDO', new Rule(['shared' => true, 'constructParams' => ['x', 'y', 'z']]));

Aside from being visually different, I'm not understanding what would be gained over the above by adding fluent setter methods, as in your example 2.

@TRPB
Copy link
Member

TRPB commented Apr 27, 2015

I'm in two minds about giving multiple syntaxes for the same configuration as it makes examples more confusing, that said I think Garrett's is the simplest solution.

In theory the Rule class could be removed entirely and just allow an array config, but imho the class gives people a quick reference for what options are available and the types they store.

I'm tempted, since we already have XML/JSON extensions to just add a new class that extends Rule

class ShorthandRule extends Rule {
    public function __construct(array $options) {
        foreach ($options as $name => $value) $this->$name = $value;
    }
}


use ShorthandRule as Rule;
use \Dice\Dice;
$dice = new Dice;
$dice->addRule('PDO'. new Rule(['shared' => true]);

This has the advantage of not needing the Dice class to understand two formats (Array and object) which is better for performance and code clarity doesn't need extra validation, doesn't break BC and exists as an optional extra.

Having said that, there's nothing to stop anyone adding the Shorthand class, or a class that implements chainable setters that works with Dice just by extending the rule class and adding whatever methods which is arguably better as it essentially allows developers to define their own syntax for their project.

@daniel-meister
Copy link
Contributor

Just to put my two cents in I really like the ShorthandRule solution.

@pine3ree
Copy link
Author

@TomBZombie
Thank You Tom. I actually implemented both constructor options and the chained setters in my derived class and modified the rule definitions with array parameters. I just needed it inside dice not to break the 1 file record!!
I understand the reason for not modifing addRule signature.
I believe though a new class rule (even if the name is appealing) is really not necessary since it doesn't add features to the basic rule other than a constructor, which basically does only initialise the object properties.
I added it like this

class Rule {
    public function __construct(array $options = []) {
        if ($options) foreach ($options as $name => $value) $this->$name = $value;
    }
//...
}

kind regards

@pine3ree
Copy link
Author

Hi @garrettw,
the only benefits of having setters are

  • value checking before assignment
  • and ensuring that correct property names are used.

But as in domain models (with private/protected properties where setters/getters are needed as an interface) this would mean a really fatter class.

best regards

@TRPB
Copy link
Member

TRPB commented Apr 27, 2015

Having thought about this some more, I'm going to do a complete 180 here and suggest the potential of the removal of the Rule class entirely. This would be very BC Breaking however:

  • Rule isn't really a class, it's a data structure
  • Not all the options are used by all instances, using an object with all the properties implies they are (I've never created a rule that uses everything at once yet)
  • An Array lends itself very easily to JSON storage of rules, the Rule class does not.
  • Going forward, the Instance class could also be removed, using an array:

$rule = new \Dice\Rule;
$rule->substitutions['C'] = new \Dice\Instance('$MyC');

becomes:

$rule = ['substitutions' => ['C' => ['instance' => '$MyC']]];

  • This then becomes 100% JSON compatible unless closures are needed
  • The Rule and Instance classes can be removed
  • The JSON loader can be heavily reduced in weight
  • And the problem this issue was raised for initially: shorter configuration code.
  • Makes Dice follow the 1 class per file rule. I resisted this before as Rule was useless without Dice.

@pine3ree
Copy link
Author

@TomBZombie
this looks even better to me! for some of us .... the fewer the better

@garrettw
Copy link

@TomBZombie I'm for whatever you think is best. You always do reason things out very thoroughly.
I'd say you could hold off on removing the classes until v2.0, and throw in the ShorthandRule or something for now.

Just for argument's sake, here's how I implemented the changed Rule constructor:

public function __construct(array $params = [])
{
    foreach ($params as $name => $val):
        $this->$name = $val;
    endforeach;
}

@TRPB
Copy link
Member

TRPB commented Apr 27, 2015

As a step further and to even better support JSON, perhaps the addRule() method should be renamed addRules.. that way you could do:

$dice->addRules(json_decode($json));

Where the JSON is something like:

{
    "class1": {
          "shared": true
     },
     "MyClass2": {
          "instanceOf": "class2"
      }
}

The only problem with this is you'd need to array_walk on the method calls to lowercase/trim the rule name but it might be simpler overall.

@pine3ree
Copy link
Author

@TomBZombie
I believe having both methods would benefit most of the cases:

  1. allowing settings for a single class in a specific part of an application, the addRule($key, $rule) is very esplicit and clear.
  2. allowing a loader-like batch settings for many classes at once (this would be most probabably used in an application bootstrap file)

...just my opinion.

best regards

@Vinai
Copy link

Vinai commented Apr 28, 2015

Chiming in here: if the rules are specified as a nested set of arrays, Dice should properly validate it at spit out a nice error message if it finds something that doesn't make sense.
As @TomBZombie wrote, the biggest benefit of the more explicit interface of the Rule class is documentation.
With proper validation Dice would still be supporting developers somewhat.

I've seen too many developer hours being wasted because of one config array key having a small typo and configuration arrays not being validated.

@TRPB
Copy link
Member

TRPB commented Apr 28, 2015

Agreed, I'd probably supply the validator as a separate class that isn't used by Dice, or a wrapper for the Dice instance, that way you'd gain performance in production and have validation during development.

@Vinai
Copy link

Vinai commented Apr 28, 2015

👍

@TRPB TRPB mentioned this issue Apr 29, 2015
@VeeeneX
Copy link
Contributor

VeeeneX commented May 8, 2015

And what about to create some simple interface on top of the Dice, something simmilar to Auryn.
Just like this:

$RollDice = new Dice\RollDice;
$RollDice ->share('PDO');
$RollDice ->define('PDO', [
    ':dsn' => 'mysql:dbname=testdb;host=127.0.0.1',
    ':username' => 'dbuser',
    ':passwd' => 'dbpass'
]);

$db = $RollDice ->make('PDO');```

@TRPB
Copy link
Member

TRPB commented May 8, 2015

That's almost as it was before, other than you're telling Dice about each option separately, I'm not sure that's really any more useful than the current method. It doesn't really help with this request for reducing the number of lines required for configuration.

@VeeeneX
Copy link
Contributor

VeeeneX commented May 8, 2015

OK, maybe it can be just an additional package for dice ;)

@TRPB
Copy link
Member

TRPB commented Aug 6, 2015

I've just released 2.0 which handles all the open issues and I've released a version for PHP5.4-5.5, see the relevant branch/release :)

@TRPB TRPB closed this as completed Aug 6, 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

No branches or pull requests

6 participants