WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 3 weeks ago

#48082 new enhancement

Replace `dirname(__FILE__)` with `__DIR__` (Performance improvement, massive patch)

Reported by: ayeshrajans Owned by:
Milestone: 5.4 Priority: normal
Severity: minor Version:
Component: General Keywords: has-patch
Focuses: performance, coding-standards Cc:
PR Number:

Description

PHP 5.3 added the __DIR__ magic constant, that is same as dirname(__FILE__). Because dirname() is a function call, this can have a performance overhead of the function call every time we use dirname(__FILE__) instead of __DIR__.

Now that we require PHP 5.6 as the minimum version, we can drop the dirname() calls in favor of __DIR__.

Attached is a massive patch that replaces all such calls with the magic constant. I have grepped the source for any further occurrences and we are clean now.

I understand reviewing such a big patch is difficult, but hopefully we can contain this set of changes within one ticket, so they are easy to merge/deal with.

Thank you.

Attachments (2)

48082-1.patch (83.8 KB) - added by ayeshrajans 5 weeks ago.
48082.diff (80.2 KB) - added by desrosj 5 weeks ago.

Download all attachments as: .zip

Change History (10)

@ayeshrajans
5 weeks ago

#1 follow-up: @jrf
5 weeks ago

  • Keywords needs-refresh added; has-patch removed
  • Version set to trunk

Hi @ayeshrajans Thanks for taking the initiative for this!

I've reviewed the patch and have a couple of remarks:

  1. In a lot of places the patch does not comply with the coding standards regarding whitespace on the inside of parentheses. Please run composer format over the patch to fix this.
  2. As nearly all of these touch include/require statements, may this be a good time to remove the unnecessary parentheses which are often used with them ?

include/require are language constructs, not function calls and not using the parentheses has an - albeit marginal - performance benefit.

<?php
// Bad:
require_once( dirname( __FILE__ ) . '/admin.php' );

// Good:
require_once dirname( __FILE__ ) . '/admin.php';

The WordPress-Extra PHPCS ruleset actually contains a sniff which already checks for this. If the unneeded parentheses would be removed, the sniff could move from the Extra ruleset to the Core ruleset. /cc @pento

  1. I wonder whether it may be more descriptive to use ABSPATH . WPINC instead of __DIR__ (where applicable). Along the same lines, the test bootstrap.php file defines a DIR_TESTROOT constant which can (and probably should) be used in quite a lot of places.
  2. I don't think files from external dependencies - such as GetID3, PHPMailer, SimplePie or Text_Diff (etc) should be patched as it may make the upgrade path in the future more difficult (checking if there are any customizations should only yield real customizations.

Other than that, this looks good.

Last edited 5 weeks ago by jrf (previous) (diff)

#2 @desrosj
5 weeks ago

  • Keywords has-patch added; needs-refresh removed
  • Version trunk deleted

Thanks for this one, @ayeshrajans!

At first glance, your patch also updates external libraries included in WordPress (SimplePie, Requests, etc.). Because these libraries are maintained elsewhere and copied verbatim into the code base as much as possible, these would need to be changed upstream before including in Core.

I have an incoming patch with those libraries skipped, and with some PHPCS fixes.

I have set up a branch on my fork and it shows the tests passing: https://travis-ci.org/desrosj/wordpress-develop/builds/587250892.

I'm tempted to tag this as 5.4 early, just in case there are any issues that may take a while to detect.

@desrosj
5 weeks ago

#3 @desrosj
5 weeks ago

  • Milestone changed from Awaiting Review to Future Release

#4 in reply to: ↑ 1 @netweb
5 weeks ago

Replying to jrf:

  1. As nearly all of these touch include/require statements, may this be a good time to remove the unnecessary parentheses which are often used with them ?

include/require are language constructs, not function calls and not using the parentheses has an - albeit marginal - performance benefit.

<?php
// Bad:
require_once( dirname( __FILE__ ) . '/admin.php' );

// Good:
require_once dirname( __FILE__ ) . '/admin.php';

The WordPress-Extra PHPCS ruleset actually contains a sniff which already checks for this. If the unneeded parentheses would be removed, the sniff could move from the Extra ruleset to the Core ruleset. /cc @pento

I think this is a great idea, two birds with one stone and all that, or in other words, it would make SVN/Git blame a little nicer doing this at the same time.

#5 @ayeshrajans
4 weeks ago

Thanks for the responses. I realized the PHPCS failures in CI, and thanks a lot for the fixes and improvements with require_once __DIR__ pattern too! +1 from me too :)

#6 @joostdevalk
4 weeks ago

Looking through this patch I'm really liking where you're going with this. Some minor feedback: I see a lot of include statements where I think we should be using require_once. For instance includes on admin-header.php don't make sense: without that the admin doesn't make any sense. Or an include on wp-load.php in xmlrpc.php, if that fails, there's no WP, a require would be much better...

#7 @jrf
4 weeks ago

I very much support @joostdevalk's suggestion.

While it may be a lot in one patch, it does make sense to me to make these changes all in one go as they all touch the same lines of code.

The sniff I previously mentioned which is in the WordPress-Extra ruleset also checks for use of unconditional include[_once] use.

To run it over WP Core while respecting the existing exclusions for the files coming from dependencies, follow the below steps:

  1. Add a new file to the root of your clone of WP Core (directory above src) called phpcs.xml.
  2. Add the following contents to that file:
<?xml version="1.0"?>
<ruleset name="WP Core local">

	<rule ref="./phpcs.xml.dist"/>

	<rule ref="PEAR.Files.IncludingFile.BracketsNotRequired">
		<type>warning</type>
	</rule>
	<rule ref="PEAR.Files.IncludingFile.UseRequire">
		<type>warning</type>
	</rule>
	<rule ref="PEAR.Files.IncludingFile.UseRequireOnce">
		<type>warning</type>
	</rule>
</ruleset>

Note: the file should automatically be git-ignored/svn-ignored.

  1. Run PHPCS with the below command to just and only see the errors coming from this sniff:
vendor/bin/phpcs --sniffs=PEAR.Files.IncludingFile

#8 @desrosj
3 weeks ago

  • Milestone changed from Future Release to 5.4

Let's prioritize this in 5.4 as part of the continued modernization/performance efforts.

Note: See TracTickets for help on using tickets.