Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

patch #5860 #5863

Closed
wants to merge 12 commits into from
Closed

Conversation

samsonasik
Copy link
Contributor

patch #5860

@@ -367,7 +367,7 @@ protected function prepareOptions(ZendLdap\Ldap $ldap, array $options)
* @param string $canonicalName
* @param string $dn
* @param array $adapterOptions
* @return string|true
* @return bool|string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this return false?

@Maks3w
Copy link
Member

Maks3w commented Feb 24, 2014

@samsonasik May is better stop a moment and share what rules are you intended to apply.

@samsonasik
Copy link
Contributor Author

@Maks3w based on #5860 , there are lot of suggested changes, but I just want to put that it appliable at zf2 and keep that it still support for consistencies, like @return self, @param bool instead of boolean like @weierophinney pointed at #3342 and #4436 (comment) . I have read the Coding Standard that pointed docblock too http://phpdoc.org and for @return type, it linked to http://phpdoc.org/docs/latest/references/phpdoc/types.html . if there is a rule that must be applied, please let me know. Thank you.

@Ocramius
Copy link
Member

The bool or boolean problem is no big deal - alignment on docblocks is likely going to conflict with some PRs, but that's also no big deal - can probably be changed in scrutinizer itself.

@return self is actually correct and supported by phpdoc - is there any problem with that?

@samsonasik
Copy link
Contributor Author

@Ocramius yes, but for consistency. btw, alignment fixed. should I continue ? /cc @Maks3w

@Ocramius Ocramius self-assigned this Feb 27, 2014
@Ocramius Ocramius closed this in d291bb5 Mar 17, 2014
@Ocramius Ocramius removed the WIP label Mar 17, 2014
@Ocramius Ocramius added this to the 2.3.1 milestone Mar 17, 2014
@Ocramius Ocramius changed the title [WIP] patch #5860 patch #5860 Mar 17, 2014
@Ocramius
Copy link
Member

Rebased and merged manually - work can go on from here IMO

@Maks3w
Copy link
Member

Maks3w commented Mar 18, 2014

@Ocramius I disagree with this. Fix may types but also remove other types allowed or expected to return.

@Ocramius
Copy link
Member

@Maks3w I agree that there may be imperfections, but the code was (in general) improved and functionally kept intact/unchanged, so I went on and merged this. I'd rather see additional fixes or cleanups (even if they fix these changes) rather than trying to slow down nitpicking (causing slowdown) @samsonasik, who has already been doing a lot for #5860 :-)

@Maks3w
Copy link
Member

Maks3w commented Mar 18, 2014

We should to have a criteria and don't change something just because some tool throws a warning. Think if the warning is correct or a false positive before change any code.

@Ocramius
Copy link
Member

@Maks3w I went through the diffs and I didn't (and still don't) see false positives tbh

Ocramius added a commit that referenced this pull request Jan 1, 2015
weierophinney pushed a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
weierophinney pushed a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
weierophinney pushed a commit to zendframework/zend-barcode that referenced this pull request May 14, 2015
weierophinney pushed a commit to zendframework/zend-barcode that referenced this pull request May 14, 2015
gianarb pushed a commit to zendframework/zend-config that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-config that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-crypt that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-crypt that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-console that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-console that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
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.

3 participants