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

drop php 5.4 check and $that = $this usage #3

Closed
wants to merge 2 commits into from

Conversation

samsonasik
Copy link
Contributor

No description provided.

*
* @param string $columnA
* @param string $columnB
*
* @return int
*/
public function compareColumnOptions($columnA, $columnB)
private function compareColumnOptions($columnA, $columnB)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead make this protected, however, this is a BC break in the event someone extended the class. You also cannot change visibility. I would revert this portion of the commit so that it can be merged to master and subsequently make this change against the develop branch as a secondary PR.

Copy link
Member

Choose a reason for hiding this comment

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

The method had been marked as @internal and @private (I think by me). Can't have more warnings than that - private is perfectly fine here, and nobody should ever have relied on it in first place.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, it was me: bf6d9fc

@mwillbanks
Copy link
Contributor

@Ocramius makes sense but still would need to be broken in a minor version vs. a bug fix release.

@samsonasik
Copy link
Contributor Author

should I change to protected, then?

Warm regards,

Abdul Malik Ikhsan

Pada 23 Jul 2015, pukul 10.23, Mike Willbanks [email protected] menulis:

@Ocramius makes sense but still would need to be broken in a minor version vs. a bug fix release.


Reply to this email directly or view it on GitHub.

@Maks3w
Copy link
Member

Maks3w commented Jul 23, 2015

For me is not a BC break. @internal means for me is not part of the public API and must not be used.

@Maks3w
Copy link
Member

Maks3w commented Jul 23, 2015

This can be merged in next maintenance release

@Maks3w Maks3w added this to the 2.5.2 milestone Jul 23, 2015
weierophinney added a commit that referenced this pull request Jul 23, 2015
weierophinney added a commit that referenced this pull request Jul 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants