Split each class in parser.php to a separate file
#48693
Conversation
|
Flaky tests detected in a5e3652. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4314694685
|
|
As a note, this was proposed in #22249 to close #21891, and was not merged. I'm certainly still of the opinion that this is a move against the health of the code even though in "the robotic viewpoint of the WordPress coding standards ruleset, [and] nothing else"[1] it's a violation. There's a harmony in the different parser implementations and the helper classes are empty constructs that have zero use or appropriate application outside of the parser class/file (as in, if we could hide them that would be ideal). If PHP allowed inner classes they would be inner classes. If PHP had record types they would be records. Unknown to me at the time #22249 was under discussion, but revealed during the Tag Processor development, moving away from classes to associate arrays incurs a somewhat major performance penalty, both in memory and compute time. Just sharing because this was debated before and never closed. Has something else changed since that time that makes this more important now? |
aristath
force-pushed
the
try/split-parser-files
branch
from
a5e3652 to
9962ff8
Compare
| // Require files. | ||
| require_once __DIR__ . '/class-wp-block-parser-block.php'; | ||
| require_once __DIR__ . '/class-wp-block-parser-frame.php'; | ||
| require_once __DIR__ . '/class-wp-block-parser.php'; |
There was a problem hiding this comment.
although I still am of the opinion that this PR is worth closing without merging, I wanted to point out that @hellofromtonya rejected HTML API work for including files like this, instead of doing everything manually in load.php/wp-settings.php
but again, I still believe it's best to exclude this file from the one-class-per-file rule because of its close correspondence to the other parser implementations and because of the the role the helper classes serve.
There was a problem hiding this comment.
@dmsnell I am not why this should be a special case. Having the one in the same file is confusing and makes it harder to find the class. It also making autoloading core classes worse as well.
There was a problem hiding this comment.
what's the problem with autoloading core classes?
I am not why this should be a special case
I've shared a few times:
- these helper classes are specifically empty classes serving as records. they contain no behavior
- they directly mirror the code in the JS and Java versions of the parser
- they have no valuable use outside of the parser and are not meant to be loaded in any other context
There was a problem hiding this comment.
WordPress/wordpress-develop#3470 (comment)
Autoloading is a project to load class files only when they are used. This saves on PHP memory, as classes are not loaded into memory that are not used. With all three classes in this one file, that is impossible.
None of the reasons you have given really make sense why we would not split these up. These is a coding standard in php for a reason, to break that standard, there has to be a benefit. I see no benefit.
There was a problem hiding this comment.
@spacedmonkey I can understand if you don't see this as a benefit, but there's a benefit there. maybe you don't value it as much, but it's not like there's no benefit.
Autoloading is a project to load class files only when they are used. This saves on PHP memory, as classes are not loaded into memory that are not used. With all three classes in this one file, that is impossible.
This just hearkens back to the reasoning I gave: there's zero purpose to ever load any of the helper classes outside the parser, and there's zero cases where the parser loads without loading the helper classes, so there's no situation in which this desired behavior would even come into play with the parser.
Are you saying that having two or more classes in a single file prevents any autoloading from working within WordPress? or that having these classes in the same file here prevents autoloading these classes?
if it's the latter then the principle has zero application to this case, other than in a purely theoretical sense. there's no memory savings by splitting these as they will always load together, and there will never be a time when they should load separately.
There was a problem hiding this comment.
@dmsnell Unit tests is a good example of where you might want to load these files apart.
In my testing WP_Block_Parser_Frame did not load on every page request.
I think we are going to have to agree to disagree on this one.
There was a problem hiding this comment.
I think we are going to have to agree to disagree on this one
Deal 👍
But let's keep it the way it is in the meantime 😄
There was a problem hiding this comment.
but again, I still believe it's best to exclude this file from the one-class-per-file rule because of its close correspondence to the other parser implementations and because of the the role the helper classes serve.
The WordPress Coding Standard is one class/interface/trait per file. This is documented in Core's Handbook. https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#only-one-object-structure-class-interface-trait-per-file
@dmsnell though I see your reasoning with close correspondence, I disagree with excluding the file from the rule. All new code should meet Core's coding standards.
There was a problem hiding this comment.
// Require files.
require_once __DIR__ . '/class-wp-block-parser-block.php';
require_once __DIR__ . '/class-wp-block-parser-frame.php';
require_once __DIR__ . '/class-wp-block-parser.php';I wanted to point out that @hellofromtonya rejected HTML API work for including files like this, instead of doing everything manually in
load.php/wp-settings.php
The loading of these files should be where all other files are loaded, and not in the parser.php file itself. Why? Consistency with how Core loads its file.
There was a problem hiding this comment.
The loading of these files should be where all other files are loaded, and not in the
parser.phpfile itself. Why? Consistency with how Core loads its file.
I agree 100%. This was done here like that for backwards-compatibility.
If a plugin or 3rd-party includes the parser.php file, it should continue to work - which is why instead of removing the file since it's no longer needed, I include the classes there 👍
|
As there is agreement for two core committers, I am going merge this change. I see no reason for these files not to follow the WordPress code standards. Having these classes in the same file, makes the code harder to find and adds an inconsistency to the codebase that is not needed. |
|
well I'm quite disappointed by this. are you planning on fixing the other coding style violations, whereas the property names are in camel case? |
I don't have any plans to do so. |
why apply the coding standards rules in some places and not others? |
This is another issue, it should be discussed in that issue. As mentioned this change is needed for another project, which is autoloading of files. Autoloading as a clear benefit to core. The other coding standards, are less important. |
as we discussed in person, I felt like a better approach could be to update the coding standards to restrict import of these internal records/classes from other files. in that sense, isn't encouraging people to import private implementation details counter to the goals of the coding standards? these classes are auto-loaded already for the parser by means of being in the same file. now we have invited people to use them where they ought not be used. maybe our auto-loader could enforce not loading these classes that are now in separate files? it would be good to throw an error if someone tries to use them. |
|
Not all these classes are used in every page request. So having them in different files means only loading them when they are needed. This is a clear benefit for performance / memory. |
What clear benefit did you measure? How much faster or lower memory was it to load these classes as part of the autoloading mechanism than having them come in with the parser which uses them? |
* Split each class to a separate file * CS fixes * CS: phpcs can't detect the filedoc
What?
The
parser.phpfile currently contains 3 classes:WP_Block_Parser_Block,WP_Block_Parser_Frame,WP_Block_ParserThis PR splits each class to its own separate file in accordance with WP Coding Standards.
See https://core.trac.wordpress.org/ticket/57832 for the Core PR that does the same thing.