Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace dirname(__FILE__) with __DIR__ #14269

Merged
merged 4 commits into from Apr 3, 2017
Merged

Replace dirname(__FILE__) with __DIR__ #14269

merged 4 commits into from Apr 3, 2017

Conversation

Spudley
Copy link
Contributor

@Spudley Spudley commented Feb 27, 2017

Pull Request for Issue # n/a.

Summary of Changes

This is a simple PR to replace dirname(__FILE__) with __DIR__ across the codebase. I excluded the vendor folder as that's third-party code, but replaced all other instances.

DIR was introduced in PHP 5.3 to reduce the need for convoluted dirname(FILE) constructs.

The change is syntactically identical, but makes the code clearer - it is shorter, easier to read, and easier to understand.

It might also make things slightly quicker, it's one less function to call. But that going to be marginal at best; the real reason for doing this is to make the code easier to work with.

Testing Instructions

This change should have zero impact on the functionality. The new code is functionally identical; just a bit shorter and neater. Testing should just be a case of making sure it still works the same.

Expected result

No change.

Actual result

No change.

Documentation Changes Required

None required.

@mbabker
Copy link
Contributor

mbabker commented Feb 27, 2017

Please revert the changes in these files.

  • administrator/components/com_joomlaupdate/restore.php is third party code
  • installation/index.php needs to be parseable on PHP 5.2 to display a "your platform isn't supported" message versus a PHP fatal due to the engine being unable to parse the file

@Spudley
Copy link
Contributor Author

Spudley commented Feb 27, 2017

@mbabker: No problem; I hadn't spotted restore.php as being third-party, but fair enough.
Re installation/index.php -- the occurrence of __DIR__ appears after the 5.3 check and won't prevent it parsing, so I don't believe this one needs to be reverted. (I also note that the main /index.php file which has the same requirement already contains references to __DIR__).

Happy to revert, but can you confirm the latter before I do so? Thanks :)

@mbabker
Copy link
Contributor

mbabker commented Feb 27, 2017

PHP will parse the entire file before executing it, it doesn't parse it line by line.

We should've not changed to using __DIR__ in the administrator/index.php or index.php files for the same reason. It's actually really inconsistent there because there's a comment that explicitly says "use define('FOO', 'bar') instead of const FOO = 'bar'".

@Spudley
Copy link
Contributor Author

Spudley commented Feb 27, 2017

Okay. I've reverted it. :)

@mbabker
Copy link
Contributor

mbabker commented Feb 27, 2017

Looks fine to me.

Whomever merges this, please merge to staging instead of the 3.8 branch. Don't see any reason why it should only apply there.

@N6REJ
Copy link
Contributor

N6REJ commented Feb 27, 2017

@mbabker me'n thomas just undid all that a year or so ago. Why are we bringing it back?

@mbabker
Copy link
Contributor

mbabker commented Feb 27, 2017

Bringing what back?

@N6REJ
Copy link
Contributor

N6REJ commented Feb 27, 2017

if you recall starting around 1.7 we removed DIR from all of J!.. was a HUGE undertaking and didn't go smoothly. Why are we reverting?

@mbabker
Copy link
Contributor

mbabker commented Feb 27, 2017

The DS constant was removed. Joomla can't remove __DIR__, it's a PHP language feature.

@N6REJ
Copy link
Contributor

N6REJ commented Feb 28, 2017

ah, thats right.. ok.. sorry for confusion 👎

@RonakParmar
Copy link

Not able to apply this PR, please see attached screen-shot.
PHP Version : 5.6.30-7+deb.sury.org~precise+1
Web Server : Apache/2.4.25 (Ubuntu)
screen shot 2017-04-03 at 04 11 36


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

@ghost
Copy link

ghost commented Apr 3, 2017

Confirming @RonakParmar Comment.

@brianteeman
Copy link
Contributor

Looks like @Spudley deleted his repository so the fork is no longer present for you to apply using patchtester

@Spudley
Copy link
Contributor Author

Spudley commented Apr 3, 2017

@brianteeman Yes I did. Sorry about that. It was a while ago now! However the patch itself is pretty trivial. If you like I'll re-create it. I guess that'll mean submitting a new PR.

@Bakual
Copy link
Contributor

Bakual commented Apr 3, 2017

I'm just going to merge by review.
The only change in a productive file is in libraries/fof/view/view.php, all other changes are in unit tests and since those passed it should be fine.

@Bakual Bakual merged commit 3f0bcd4 into joomla:3.8-dev Apr 3, 2017
@Bakual Bakual added this to the Joomla 3.7.0 milestone Apr 3, 2017
@mbabker mbabker modified the milestones: Joomla 3.8.0, Joomla 3.7.0 Apr 3, 2017
@Spudley
Copy link
Contributor Author

Spudley commented Apr 3, 2017

Thank you @Bakual. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants