User tests: Successful: Unsuccessful:
This PR fixes misuses of array_push().
Since we're avoiding a function call, a bit of optimization is achieved.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration Components Templates (admin) Layout Libraries External Library |
Labels |
Added:
?
?
|
Labels |
Added:
?
?
|
OK, will do.
Labels |
Added:
?
|
@frankmayer do you want to send the changes to the framework or should I do with credits to here?
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
.
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?
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
.
@frankmayer fof has a repo is you want to contribute your suggestions for fof there, Joomla 3.x includes the 2.x branch.
yes, thank you @photodude, I'll do that.
Labels |
Removed:
?
|
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?
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.
@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).
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?
So ultimately what justifies accepting modifications to a third party library?
IMHO ... Nothing.
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???
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 |
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 |
Fixed some conflicts that had occurred in the meantime. It's essentially ready to be merged, when the team finds the time.
I have tested this item
code review
I have tested this item
Code review.
Status | Pending | ⇒ | Ready to Commit |
Labels |
RTC
Milestone |
Added: |
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:
?
|
Thanks for your PR @frankmayer but please revert all changes in
libraries/vendor/
andlibraries/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)