? Pending

User tests: Successful: Unsuccessful:

avatar mickmackusa
mickmackusa
17 Oct 2020

There is no issue number associated with these changes
This is my attempt to contribute to the project by improving the coding standard as a Joomla non-expert.
I don't honestly care how much of my stuff gets included into the core, but I just want to give my 2 cents and hope that decision makers will offer constructive feedback so that I can improve my contributions.

Summary of Changes

I have done a full-file review of just two randomly selected files. The intention is to make no behavioural changes. The changes are based on a range of benefits, such as:

  1. improving code performance (replacing regex with non-regex techniques)
  2. eliminating single-used variables and unnecessary overheads
  3. replacing old array() syntax with modern [] and isset() calls with null coalescing
  4. improving regex performance and reducing the data in the matches output / replacement arguments (several uses of \K)
  5. removing unnecessary function calls (like empty() when a variable is assured to be declared/ then just checking for a falsey value)
  6. making code more stable (2nd param on preg_quote() call)
  7. remove anti-patterns (such as switch(true) block)
  8. unset() can receive multiple arguments

Testing Instructions

none

Actual result BEFORE applying this Pull Request === Expected result AFTER applying this Pull Request

Documentation Changes Required

none

avatar mickmackusa mickmackusa - open - 17 Oct 2020
avatar mickmackusa mickmackusa - change - 17 Oct 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 17 Oct 2020
Category Repository Front End Plugins
avatar brianteeman
brianteeman - comment - 17 Oct 2020

this should be done for 4,1 as its too late in the release cycle for 4.0 (my personal opinion only)

avatar HLeithner
HLeithner - comment - 17 Oct 2020

@mickmackusa please create a PR of the bump.php changes against staging, I will look at it then.

As george already said don't do array conversation in j4 if not needed because it makes j3->j4 merges harder.

About the Switch to if conversion you can do this but I think with have 100 of these constructs. I don't see a good reason to waste time on this because we have much more important code to fix.

I'm closing this PR and hope you create a new one at least for bump.php at staging branch.

Thanks for you engagement.

avatar HLeithner HLeithner - change - 17 Oct 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-10-17 11:21:03
Closed_By HLeithner
Labels Added: ?
avatar HLeithner HLeithner - close - 17 Oct 2020

Add a Comment

Login with GitHub to post a comment