? Success

User tests: Successful: Unsuccessful:

avatar frankmayer
frankmayer
20 Dec 2016

Summary of Changes

  • Type-safe string comparison in site/templates

The changes in this PR should be fairly easy to review. They are only type safe string comparisons. This PR is done, so the batch PR #12233 can become a little lighter and easier to review. In hope that this will get merged quickly so further work can be done without conflicting with other PRs. ;)

Testing Instructions

None, should not change behavior

Documentation Changes Required

None.

avatar frankmayer frankmayer - open - 20 Dec 2016
avatar frankmayer frankmayer - change - 20 Dec 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Dec 2016
Category Front End Templates (site)
avatar laoneo
laoneo - comment - 21 Dec 2016

I really appreciate your work here cleaning up the Joomla code. But it produces a lot of merge conflicts in other pull requests and long running projects like the Joomla 4 template. I think we need here a strategy how to deal with that situation.

avatar mbabker
mbabker - comment - 21 Dec 2016

Every pull request has the potential to cause merge conflicts with any other active project, no matter the strategy (unless the strategy is to not accept PRs like these) it's just going to cause someone a headache.

avatar laoneo
laoneo - comment - 21 Dec 2016

@mbabker I know that every merge has a potential of a conflict. I'm also not asking to stop that (despite the fact that there are automated tools which can do that), but it should be done with some concept. It can't be that I have to check and fix for conflicts every day in my PR's.

avatar frankmayer
frankmayer - comment - 21 Dec 2016

@laoneo I know... I've been dealing with the conflicts, too, as I try to lighten up the big PR's that I made initially, so they can get reviewed easier and finally merged.

Do you have some concept in mind?
One scenario that comes to mind, is to get all those big change-PR's merged as soon as possible, and then give people the green light to resolve their conflicts in one go. Or the other way around...

However, that being said, there is still a lot more to be cleaned up and optimized.

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Dec 2016

just an idea,
mantainers could mark a day of the week to merge this type of PR (but would still depend on them being tested/reviewed), then instead of fixing conflicts everyday, you just need to do it one day per week (if aplicable).
When i say week it could be 10 days or any other period.

avatar laoneo
laoneo - comment - 21 Dec 2016

I would rather do them for Joomla 4 and leave 3 as it is. Then we can think of doing automated in one shot.

avatar laoneo
laoneo - comment - 21 Dec 2016

After that we can make then our CS rules more strictly across the whole project.

avatar frankmayer
frankmayer - comment - 21 Dec 2016

A lot of the changes that I have done are semi-automated and with lots of manual double-checking. It doesn't work fully automated. Neither the changes themselves nor the conflict resolutions.

I think that those changes should already be done the earliest possible currently Joomla 3.x and not wait for version 4. It is better to have a cleaner start into Joomla 4 than to release Joomla 4 and then change all this stuff...
And surely either being in 3.x or in later in 4, conflicts will always be a pain, when such drastic changes are introduced. There will always be long running PR's that will need conflict resolution.

avatar mbabker
mbabker - comment - 21 Dec 2016

In general, you want to try and not have too many code style/structure changes between branches, it makes branch merging much more painful when you have those differences on top of regular patches. It's in part why Symfony turns down PRs for using PHP 5.5 structures in code that existed in Symfony 2.x (newer code can use things like short array syntax and the ::class constant).

Now if we do the same thing as we did with 2.5 and 3.0 and never merge things forward (basically maintaining both branches as separate things at that point), it becomes a moot point.

avatar frankmayer frankmayer - change - 21 Dec 2016
Labels Added: ?
avatar frankmayer
frankmayer - comment - 21 Dec 2016

BTW, conflicts resolved for this one... :D

avatar wilsonge wilsonge - change - 21 Dec 2016
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-12-21 20:05:36
Closed_By wilsonge
avatar wilsonge wilsonge - close - 21 Dec 2016
avatar wilsonge wilsonge - merge - 21 Dec 2016
avatar wilsonge wilsonge - reference | 7e710f8 - 21 Dec 16
avatar wilsonge wilsonge - merge - 21 Dec 2016
avatar wilsonge wilsonge - close - 21 Dec 2016
avatar wilsonge wilsonge - change - 21 Dec 2016
Milestone Added:
avatar frankmayer frankmayer - head_ref_deleted - 25 Dec 2016

Add a Comment

Login with GitHub to post a comment