? Success

User tests: Successful: Unsuccessful:

avatar frankmayer
frankmayer
15 Jun 2017

Summary of Changes

Implemented type-safe comparisons in the libraries / joomla directory.
For easier reviewing, there is a total of five parts, that cover the libraries / joomla directory

This is part 1, which covers directories access until crypt

Testing Instructions

Code review only, as those changes should not affect behavior.
Despite the quantity of the changes, it should be fairly easy to review.

Attention to code reviewers:
In order to prevent code-review-fatigue, I only did type safe comparisons, without applying:

  • formatting
  • removal of unnecessary parentheses
  • other non-related changes

Please only review the correctness of the specific changes, without pointing out any possible formatting issues (unless corrupted by this PR), removal of parentheses and other non-related changes. This will help get this PR reviewed and merged quickly.
If you should find other prospects for type safe comparison in those folders please note them in a review.

Thank you

avatar frankmayer frankmayer - open - 15 Jun 2017
avatar frankmayer frankmayer - change - 15 Jun 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Jun 2017
Category Libraries
avatar frankmayer frankmayer - change - 15 Jun 2017
The description was changed
avatar frankmayer frankmayer - edited - 15 Jun 2017
avatar frankmayer frankmayer - change - 15 Jun 2017
The description was changed
avatar frankmayer frankmayer - edited - 15 Jun 2017
avatar frankmayer
frankmayer - comment - 15 Jun 2017

Bump @andrepereiradasilva & @Quy up for the challenge? ;)
(Important: Check code review instructions first.)

avatar frankmayer frankmayer - change - 16 Jun 2017
The description was changed
avatar frankmayer frankmayer - edited - 16 Jun 2017
avatar laoneo
laoneo - comment - 16 Jun 2017

Thank you for your effort making Joomla better. But you have to consider that we start namespacing the libraries classes in the Joomla 3.8 branch. So I suggest to make this kind of pr's against the namespaced classes in 3.8. Keep also in mind that some libraries will get removed in Joomla 4 and some replaced with the framework packages. I suggest you coordinate your efforts with the release leader @wilsonge.

avatar frankmayer
frankmayer - comment - 16 Jun 2017

Thank you for that valuable info. I will look into that.

avatar frankmayer
frankmayer - comment - 16 Jun 2017

@wilsonge what's the way to go here? How to coordinate efforts on this?

avatar frankmayer
frankmayer - comment - 16 Jun 2017

Closing this set of PR's in favor of opening a new one against 3.8-dev (or 4.0?). We'll see.

avatar frankmayer frankmayer - change - 16 Jun 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-06-16 09:41:48
Closed_By frankmayer
Labels Added: ?
avatar frankmayer frankmayer - close - 16 Jun 2017
avatar Bakual
Bakual - comment - 16 Jun 2017

I would postpone those kind of PRs for the time after 4.0 is released and things have settled.

avatar frankmayer
frankmayer - comment - 16 Jun 2017

Yes, propably. But I would also like to hear @wilsonge's opinion or suggestion on the matter.

avatar mbabker
mbabker - comment - 16 Jun 2017

I would postpone those kind of PRs for the time after 4.0 is released and things have settled.

Why would you suggest telling someone to wait 6-12 months to do some PRs? They're fine to propose at any time.

For stuff that has a corresponding Framework package, the PR can be done to the Framework repos at any time. For the stuff not being replaced with Framework packages, do the PRs against the 3.8 branch and it should be fine.

avatar Bakual
Bakual - comment - 16 Jun 2017

Why would you suggest telling someone to wait 6-12 months to do some PRs? They're fine to propose at any time.

For the reason that it will create conflicts with other PRs done and some of the work may even be superfluous. And there is no hurry for those kind of PRs to get in as it doesn't specifially solve a bug. It's like codestyle PRs which I also wouldn't do at the current time (both for my own sanity and others).
But then I'm not the one who has to solve the merge conflicts and wastes time by improving things in files that get removed/replaced soon. So if you're fine with it, go ahead ?

avatar mbabker
mbabker - comment - 16 Jun 2017

Every PR has the chance of conflicting with someone else's work. It's just the nature of a collaborative process like this. Ya, some of them are more disruptive than others, but that's when you start weighing the benefits versus the issues.

Code style changes for the sake of it, after doing it and regretting it, are the ones I really suggest to avoid as that is usually the main thing that makes branch merges a living hell. Changes like these, while disruptive, also come with a practical benefit of making the API a bit more strict and helps to identify possibly bad code or logic (unless you buy into removing visual debt), at least from my perspective it's worth going through the pain of whatever conflicts that will introduce.

avatar frankmayer
frankmayer - comment - 16 Jun 2017

Changes like these, while disruptive, also come with a practical benefit of making the API a bit more strict and helps to identify possibly bad code or logic (unless you buy into removing visual debt), at least from my perspective it's worth going through the pain of whatever conflicts that will introduce.

... and that is the reason I did this in the first place. And while doing it, I found some inconsistencies, which I was going to document first and possibly investigate later.

The whole thing depends on when Joomla 4.0 will really be released. If it is released after 2 years, then I'd say the effort is worth it. If it is release in the next six months, one would probably need to remove the changes concerning removed libraries and pick only the ones that will move into Joomla 4.0.

Is there somewhere a plan on what is being re(moved) or is it safe to just check the @deprecated tags?

avatar brianteeman
brianteeman - comment - 16 Jun 2017

whenever joomla 4 is released joomla 3 will be supported for a further two years

avatar mbabker
mbabker - comment - 16 Jun 2017

Deprecation tags and https://github.com/joomla/joomla-cms/projects/5 are a good pointer for what's being dropped fully and what's being transitioned to using Framework packages. Aside from that right now there isn't a master plan.

Add a Comment

Login with GitHub to post a comment