? ? Success

User tests: Successful: Unsuccessful:

avatar frankmayer
frankmayer
25 Sep 2016

This PR fixes misuses of array_push().

Since we're avoiding a function call, a bit of optimization is achieved.

avatar frankmayer frankmayer - open - 25 Sep 2016
avatar frankmayer frankmayer - change - 25 Sep 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Sep 2016
Category Administration Components Templates (admin) Layout Libraries External Library
avatar joomla-cms-bot joomla-cms-bot - change - 25 Sep 2016
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 25 Sep 2016
Labels Added: ? ?
avatar zero-24
zero-24 - comment - 25 Sep 2016

Thanks for your PR @frankmayer but please revert all changes in libraries/vendor/ and libraries/fof/ that is 3rd Party Code that is managed outside of the CMS repo.

There are also the rand / mt_rand changes included here i guess this is not expected?

The upstream repos for the libraries/vendor/Joomla folder are the framework (https://github.com/joomla-framework)

avatar frankmayer
frankmayer - comment - 25 Sep 2016

OK, will do.

avatar joomla-cms-bot joomla-cms-bot - change - 25 Sep 2016
Labels Added: ?
avatar zero-24
zero-24 - comment - 25 Sep 2016

@frankmayer do you want to send the changes to the framework or should I do with credits to here?

avatar frankmayer
frankmayer - comment - 25 Sep 2016

Thanks @zero-24, I will do that in a while. Preparing a few more optimizations, right now.
BTW, how long approx. do those PR's take until they are merged into staging (if ok)?

avatar mbabker
mbabker - comment - 25 Sep 2016

It depends on the package and when a release is tagged. Typically we don't
tag a new release without having a few changes queued up unless it's a
priority.

On Sunday, September 25, 2016, Frank Mayer notifications@github.com wrote:

Thanks @zero-24 https://github.com/zero-24, I will do that in a while.
Preparing a few more optimizations, right now.
BTW, how long approx. do those PR's take until they are merged into
staging (if ok)?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#12170 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoZNLvXx4W2JOwlpgGW4nUeRvWjQDks5qtq8UgaJpZM4KF7Oe
.

avatar frankmayer
frankmayer - comment - 25 Sep 2016

OK, thanks @mbabker , I am asking, because there are a few more PR's that are essentially based on my previous PR's, so the question is, should I make those small PR's for you guys not to loose overview, or huge ones with lots of commits, in order to have the related changes altogether?

avatar mbabker
mbabker - comment - 25 Sep 2016

Usually smaller ones are easier to review and merge quicker so if you do
one for each type of change that should be enough on the CMS repo. For the
Framework I usually merge much quicker but the time it takes to tag a
release and inherently get updated in the CMS can be longer since there
isn't a fixed schedule for tagging releases there and it's largely
dependent on what changes are in the queue compared to the last tag (in a
handful of cases the only changes are to the testing environment so no
reason to make a new release).

On Sunday, September 25, 2016, Frank Mayer notifications@github.com wrote:

OK, thanks @mbabker https://github.com/mbabker , I am asking, because
there are a few more PR's that are essentially based on my previous PR's,
so the question is, should I make those small PR's for you guys not to
loose overview, or huge ones with lots of commits, in order to have the
related changes altogether?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12170 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoerZrVRsIKJF54TZkvdE7hnxdaBYks5qtrDigaJpZM4KF7Oe
.

avatar frankmayer
frankmayer - comment - 25 Sep 2016

Ok, thanks.
BTW, the framework had already a different implementation, not needing my changes anymore. It looks like @mbabker has done a lot of cleaning up and improving, there.

avatar photodude
photodude - comment - 26 Sep 2016

@frankmayer fof has a repo is you want to contribute your suggestions for fof there, Joomla 3.x includes the 2.x branch.

avatar frankmayer
frankmayer - comment - 26 Sep 2016

yes, thank you @photodude, I'll do that.

avatar zero-24 zero-24 - change - 26 Sep 2016
Labels Removed: ?
avatar photodude
photodude - comment - 6 Oct 2016

Reading from another issue and from the related fof issue "You can ask the Joomla! project to include that change too.... FOF 2 is end of life now and won't release another numbered version." and "Right now FOF 2 isn't supported anymore. So any change you make into the copy in J! 3 won't be a problem."

Do we want to bring back the changes for the FOF portion?

avatar mbabker
mbabker - comment - 6 Oct 2016

Even after I've pointed that out PLT has rejected most changes related to FOF. Which I find kind of funny because we have altered so many other third party libraries I don't get what makes this one special.

avatar photodude
photodude - comment - 6 Oct 2016

@mbabker, when fof2 was first included I could understand not modifying it and submitting changes to the official repository, but now that FOF2 EOL and the developer has explicitly stated that we now have a green light for directly making the changes to the version included in joomla I no longer understand rejecting such changes (under the specific guidelines that have been outlined for this exception).

avatar mbabker
mbabker - comment - 7 Oct 2016

We've hacked supported libraries too. When 3.0.0 released with Bootstrap 2.1 (current at the time) it contained hacks. So as a project we're not above hacking libraries, maintained or otherwise. That's why I have a hard time understanding what hacks are acceptable and what hacks are not. We have the changes in Bootstrap, 3.7 has the current version of Chosen with its internals hacked to use the 0.x selectors, and the password compat polyfill had hacks in it at 3.2 (since reverted thankfully).

So ultimately what justifies accepting modifications to a third party library?

avatar dgt41
dgt41 - comment - 7 Oct 2016

@mbabker I think I've fixed the js/css for 4 and provided a workflow that will prevent such misbehaviours in the future (hopefully 😄)

avatar andrepereiradasilva
andrepereiradasilva - comment - 7 Oct 2016

So ultimately what justifies accepting modifications to a third party library?

IMHO ... Nothing. 😄

avatar photodude
photodude - comment - 7 Oct 2016

I find it sad that as a project we constantly tell people not to do "core hacks", yet we are guilty of making core hacks to actively maintained third party libraries. Then when a third party library goes EOL and the maintainer says "the ball is in your court" we may or may not reject making any fixes and we also don't replace the library with a current maintained version complaining (rightly or wrongly) about BC issues with making the change.

So ultimately what justifies accepting modifications to a third party library?

I agree that 99.99% of the time we should not be making and accepting modifications to a third party libraries. Any exceptions to that should have significant weight to justify making the change (temporary security related or EOL libraries that have significant reasons not to update to a current version)

But how do we deal with the current mixed messages and mixed attitudes relating to how we have maintained and modified third party libraries???

avatar frankmayer frankmayer - change - 12 Dec 2016
The description was changed
avatar joomla-cms-bot joomla-cms-bot - change - 12 Dec 2016
Category Administration Components Templates (admin) Layout Libraries External Library Administration com_categories com_installer com_joomlaupdate com_menus Templates (admin) Layout Libraries MS SQL Front End Plugins Unit Tests Components
avatar joomla-cms-bot joomla-cms-bot - change - 12 Dec 2016
Category Administration Components Templates (admin) Layout Libraries com_categories com_installer com_joomlaupdate com_menus MS SQL Front End Plugins Unit Tests Administration com_categories com_installer com_joomlaupdate com_menus Templates (admin) Layout Libraries MS SQL Unit Tests Components
avatar frankmayer
frankmayer - comment - 12 Dec 2016

Fixed some conflicts that had occurred in the meantime. It's essentially ready to be merged, when the team finds the time.

avatar andrepereiradasilva
andrepereiradasilva - comment - 12 Dec 2016

I have tested this item successfully on 3dc16d6

code review


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12170.

avatar andrepereiradasilva andrepereiradasilva - test_item - 12 Dec 2016 - Tested successfully
avatar csthomas
csthomas - comment - 13 Dec 2016

I have tested this item successfully on 3dc16d6

Code review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12170.

avatar csthomas csthomas - test_item - 13 Dec 2016 - Tested successfully
avatar jeckodevelopment jeckodevelopment - change - 13 Dec 2016
The description was changed
Status Pending Ready to Commit
Labels
avatar jeckodevelopment
jeckodevelopment - comment - 13 Dec 2016

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12170.

avatar jeckodevelopment jeckodevelopment - edited - 13 Dec 2016
avatar jeckodevelopment jeckodevelopment - change - 13 Dec 2016
Milestone Added:
avatar rdeutz rdeutz - reference | 4e13f90 - 13 Dec 16
avatar rdeutz rdeutz - merge - 13 Dec 2016
avatar rdeutz rdeutz - close - 13 Dec 2016
avatar rdeutz rdeutz - change - 13 Dec 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-12-13 12:15:07
Closed_By rdeutz
Labels Added: ?
avatar rdeutz rdeutz - close - 13 Dec 2016
avatar rdeutz rdeutz - merge - 13 Dec 2016
avatar frankmayer frankmayer - head_ref_deleted - 13 Dec 2016
avatar cpfeifer cpfeifer - reference | fb3e6ba - 22 Dec 16

Add a Comment

Login with GitHub to post a comment