[ZF3] Clean Stdlib #5230

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
9 participants
@bakura10
Contributor

bakura10 commented Oct 5, 2013

  • Remove fluent interface for options class
  • Remove the Hydrator classes from Stdlib as it will have its own component (zendframework#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.

+ if (!$this->offsetExists($key)) {
+ return $ret;
+ }
+ $ret =& $this->storage[$key];

This comment has been minimized.

@samsonasik

samsonasik Oct 5, 2013

Contributor

space after = ?

@samsonasik

samsonasik Oct 5, 2013

Contributor

space after = ?

This comment has been minimized.

@bakura10

bakura10 Oct 5, 2013

Contributor

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

@bakura10

bakura10 Oct 5, 2013

Contributor

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

@Thinkscape

This comment has been minimized.

Show comment
Hide comment
@Thinkscape

Thinkscape Oct 5, 2013

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.

Member

Thinkscape commented Oct 5, 2013

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

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 5, 2013

Member

Zf3 IS 5.4+ ;)
On 5 Oct 2013 20:18, "Artur Bodera" notifications@github.com wrote:

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.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/5230#issuecomment-25753829
.

Member

Ocramius commented Oct 5, 2013

Zf3 IS 5.4+ ;)
On 5 Oct 2013 20:18, "Artur Bodera" notifications@github.com wrote:

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.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/5230#issuecomment-25753829
.

@bakura10

This comment has been minimized.

Show comment
Hide comment
@bakura10

bakura10 Oct 5, 2013

Contributor

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).

Contributor

bakura10 commented Oct 5, 2013

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

This comment has been minimized.

Show comment
Hide comment
@Thinkscape

Thinkscape Oct 5, 2013

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.

Member

Thinkscape commented Oct 5, 2013

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.

@@ -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

This comment has been minimized.

@Maks3w

Maks3w Oct 6, 2013

Member

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

@Maks3w

Maks3w Oct 6, 2013

Member

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

This comment has been minimized.

@bakura10

bakura10 Oct 6, 2013

Contributor

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

@bakura10

bakura10 Oct 6, 2013

Contributor

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

This comment has been minimized.

@Maks3w

Maks3w Oct 6, 2013

Member

I'm not sure too

@Maks3w

Maks3w Oct 6, 2013

Member

I'm not sure too

This comment has been minimized.

@samsonasik

samsonasik Oct 6, 2013

Contributor

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()
@samsonasik

samsonasik Oct 6, 2013

Contributor

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()

This comment has been minimized.

@bakura10

bakura10 Oct 6, 2013

Contributor

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.

@bakura10

bakura10 Oct 6, 2013

Contributor

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.

This comment has been minimized.

@samsonasik

samsonasik Oct 6, 2013

Contributor

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

@samsonasik

samsonasik Oct 6, 2013

Contributor

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

This comment has been minimized.

@marc-mabe

marc-mabe Dec 22, 2014

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.

@marc-mabe

marc-mabe Dec 22, 2014

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.

@weierophinney weierophinney added this to the 3.0.0 milestone Mar 3, 2014

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

@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 😄

@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 😄

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

Can you just remove the @return annotation?

Can you just remove the @return annotation?

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

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

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

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

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

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

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

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

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

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

Needs a sprintf or double quotes

Needs a sprintf or double quotes

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

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

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

@jamiehannaford

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

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.

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

This comment has been minimized.

Show comment
Hide comment
@jamiehannaford

jamiehannaford May 19, 2014

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

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

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius May 19, 2014

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

Member

Ocramius commented May 19, 2014

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

This comment has been minimized.

Show comment
Hide comment
@mtymek

mtymek Jul 25, 2014

Contributor

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

Contributor

mtymek commented Jul 25, 2014

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

- $this->assertSame($options, $options->setFromArray($array));
+ $this->assertSame($options, $options);

This comment has been minimized.

@Maks3w

Maks3w Oct 21, 2015

Member

Is this a joke? :)

@Maks3w

Maks3w Oct 21, 2015

Member

Is this a joke? :)

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 21, 2015

Member

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

Member

Maks3w commented Oct 21, 2015

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

@Maks3w Maks3w closed this Oct 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment