Skip to content

Loading…

[ZF3] Clean Stdlib #5230

Closed
wants to merge 2 commits into from

9 participants

@bakura10
  • Remove fluent interface for options class
  • Remove the Hydrator classes from Stdlib as it will have its own component (#5000)
  • Remove deprecated classes
  • Updated tests
  • Remove InitializableInterface because this interface was introduced for doing bad things (the infamous init method).
  • CallbackHandler class is a bigger refactoring (see this PR).

Some notes while checking the code:

  • I don't remember the status about ArrayObject. I thought we wanted to throw it out no?

Don't hesitate to quickly check this namespace. It's pretty quick and most classes are small.

@samsonasik samsonasik commented on the diff
library/Zend/Stdlib/ArrayObject.php
((298 lines not shown))
+ return isset($this->storage[$key]);
+ }
+
+ /**
+ * Returns the value at the specified key
+ *
+ * @param mixed $key
+ * @return mixed
+ */
+ public function &offsetGet($key)
+ {
+ $ret = null;
+ if (!$this->offsetExists($key)) {
+ return $ret;
+ }
+ $ret =& $this->storage[$key];

space after = ?

@bakura10
bakura10 added a note

I think yes. I was afraid of changing this because I thought it was a hidden bit operator I didn't know :o

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Thinkscape
Zend Framework member

Remove fluent interface for options class

What's wrong with it ?

Remove deprecated classes

Why do you think they are deprecated ?

Btw: AFAIK we have not yet settled on the min. PHP version for zf3. This will determine the scope of potential changes.

@Ocramius
Zend Framework member
@bakura10

What's wrong with it ?

Because @Ocramius hate them and have valid reasons. And because @Ocramius is always right =).

Why do you think they are deprecated ?

Two classes were explicitly marked as deprecated in the code, one class was made only for PHP 5.3.3 as a fix.

Btw: AFAIK we have not yet settled on the min. PHP version for zf3. This will determine the scope of potential changes.

PHP 5.4 for ZF3 :). It has been decided (well, not officially but everyone agree to this).

@Thinkscape
Zend Framework member

Because @Ocramius hate them and have valid reasons. And because @Ocramius is always right =).

I couldn't care less :-) Unless there's a good reason, it shouldn't be removed.

Two classes were explicitly marked as deprecated in the code, one class was made only for PHP 5.3.3 as a fix.

Ah, those, yeah.

PHP 5.4 for ZF3 :). It has been decided (well, not officially but everyone agree to this).

Yeah, it's 5.4 or 5.5. If we're aiming mid/late 2014, then it might be 5.5. Just remember that 5.4.* also has some bugs shared with 5.3.*, so be careful.

@Maks3w Maks3w commented on the diff
library/Zend/Stdlib/AbstractOptions.php
@@ -91,16 +94,18 @@ public function toArray()
*/
public function __set($key, $value)
{
- $setter = 'set' . str_replace(' ', '', ucwords(str_replace('_', ' ', $key)));
+ $setter = 'set' . str_replace('_', '', $key); // PHP is case insensitive for methods
@Maks3w Zend Framework member
Maks3w added a note

But could be a magic method and the method name become an argument

@bakura10
bakura10 added a note

Mmhh didn't think of that. And that would be a problem (just asking) ?

@Maks3w Zend Framework member
Maks3w added a note

I'm not sure too

I'm comment on "// PHP is case insensitive for methods" comment, maybe the mean is "// PHP is NOT case insensitive for methods" ?

function foo()
{

}

function FOO()
{

}

got fatal error :

PHP Fatal error:  Cannot redeclare FOO()
@bakura10
bakura10 added a note

It IS case insensitive when you call it:

function FOO()
{
   echo 'hi';
}

foo(); // prints "hi"

The fact that you cannot redeclare it proves it that it IS case insensitive :). Otherwise it would allow you to declare with differnet name.

ops, sorry, my bad english ^^ , I forgot "in" means ^^, sorry sorry sorry ^^

@marc-mabe Zend Framework member

PS: This optimization for case-insensitive method names has already be merged. There is no issue with the magic method __call because this method check availability using method_exists - so __call is never be called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney added this to the 3.0.0 milestone
@jamiehannaford

@Thinkscape Because fluent interfaces break encapsulation. The only use case for having them is to reduce the verbosity of, say, query building. IMHO, for normal interfaces they conflate logic:

every method should either be a command that performs an action, or a query that returns data to the caller, but not both.

Uncle Bob also likens method chaining to train wrecks, which I agree with :smile:

@jamiehannaford

Can you just remove the @return annotation?

@jamiehannaford

Just asking - but is the convention always to format these property docblocks as:

/**
 * @var array Lorem ipsum 
 */

rather than:

/** @var array Lorem ipsum */

You can probably save a lot of lines in the lib

@jamiehannaford

How strictly will ZF3 enforce PSR-2 line limits? Maybe we can split long signatures on multiple lines

Probably also makes sense add array type hint for $input too

@jamiehannaford

Needs a sprintf or double quotes

@jamiehannaford

Should all state changes be done through dedicated setter methods - regardless of whether it's done internally or externally? Just a thought

@jamiehannaford

Would it make more sense to encapsulate SPL interface-specific functionality in traits, rather than top-level abstract classes? You'd be allowing concrete classes to compose their own functionality then, instead of forcing them to inherit a whole load of (possibly unwanted) stuff.

@jamiehannaford

Question: should Message be in the standard lib? I don't think it belongs here - it's a HTTP-specific entity. I know Zend\Console relies on it, but perhaps it should explicitly rely on Zend\Http

@Ocramius
Zend Framework member

I couldn't care less :-) Unless there's a good reason, it shouldn't be removed.

@Thinkscape sorry for not hopping in earlier. I've blogged extensively on why fluent interfaces are a problem

@mtymek

@bakura10 did you also consider moving StringUtils away from StdLib?

@Maks3w Maks3w commented on the diff
tests/ZendTest/Stdlib/OptionsTest.php
((8 lines not shown))
- $this->assertSame($options, $options->setFromArray($array));
+ $this->assertSame($options, $options);
@Maks3w Zend Framework member
Maks3w added a note

Is this a joke? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w
Zend Framework member

All of this is done stdlib or there are PRs proposing the same except removal of Fluent interfaces.

@Maks3w Maks3w closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 5, 2013
  1. @bakura10

    Clean stdlib

    bakura10 committed
  2. @bakura10

    Change back test path

    bakura10 committed
Something went wrong with that request. Please try again.