? Success

User tests: Successful: Unsuccessful:

avatar rdeutz
rdeutz
16 Nov 2015

Reverts joomla/joomla-cms#7782

We have to revert the change because a protected function _fromObject and _sortObject are removed by the change.

Here is why. Someone could have extend the class and use one of the removed functions. It is a bad idea but someone could and this change isn't B/C.

avatar rdeutz rdeutz - open - 16 Nov 2015
avatar rdeutz rdeutz - change - 16 Nov 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Nov 2015
Labels Added: ?
avatar wilsonge
wilsonge - comment - 17 Nov 2015

At least keep the changes in toInteger and arrayUnique they are fine and have no b/c implications

avatar rdeutz
rdeutz - comment - 17 Nov 2015

@wilsonge Let us merge this one and then re-think what we can change

avatar wilsonge
wilsonge - comment - 17 Nov 2015

Why? there a 4 function changes. 2 of them are b/c 2 aren't. so just revert the stuff that is not b/c.

avatar izharaazmi
izharaazmi - comment - 17 Nov 2015

@wilsonge @rdeutz Should I PR again to revert just those methods with B/C issues?

I mean to revert only fromObject , _fromObject , sortObjects , _sortObjects. And leave toInteger , pivot, arrayUnique methods as it is now.

If we can update ArrayHelper form the https://github.com/joomla-framework/utilities, then it would still be possible to minimize the methods fromObject , _fromObject. Otherwise it is important to revert these two.

avatar wilsonge
wilsonge - comment - 17 Nov 2015

We're not going to change the framework. We'll just need to override those methods at the CMS levels. But yes - we'll need to keep the _fromObject and _sortObjects methods

avatar izharaazmi
izharaazmi - comment - 17 Nov 2015

@wilsonge I didn't mean to change the framework.
I just notices that in the 3.5 beta release the Joomla\Utilities\ArrayHelper is not up to date with the framework repository. Therefore it was just a humble reminder. :-)

avatar rdeutz
rdeutz - comment - 17 Nov 2015

I would prefer to revert this one and have a new PR for the changes that are not a B/C break, could you do a PR @izharaazmi ?

avatar izharaazmi
izharaazmi - comment - 17 Nov 2015

@rdeutz Sure! But its over for today's work now. I'll do that tomorrow positively.

avatar izharaazmi
izharaazmi - comment - 18 Nov 2015

@rdeutz I am ready to send the PR let me know if you are merging this reverse or I should just include this all in my new PR.

avatar izharaazmi izharaazmi - reference | 3a8878e - 18 Nov 15
avatar roland-d roland-d - change - 18 Nov 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-11-18 19:03:55
Closed_By roland-d
avatar roland-d roland-d - close - 18 Nov 2015
avatar roland-d roland-d - reference | e122e3f - 18 Nov 15
avatar roland-d roland-d - merge - 18 Nov 2015
avatar roland-d roland-d - close - 18 Nov 2015
avatar rdeutz rdeutz - head_ref_deleted - 18 Nov 2015
avatar zero-24 zero-24 - change - 18 Nov 2015
Milestone Added:

Add a Comment

Login with GitHub to post a comment