-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Plugin: Document the project structure and conventions used in the lib
folder for PHP code
#39603
Conversation
are not ready to be merged to Core. | ||
- `lib/stable` - Stable features that exist only in the plugin. They could one | ||
day be merged to Core, but not yet. | ||
- `lib/compat/wordpress-X.Y` - Stable features that are intended to be merged to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not suggesting we do it right away, but do you think it'd be helpful for folks if expanded the compat topic at some stage? For example rationale, preferred naming conventions when extending classes, things to test, and so on.
There are also plenty of lessons to draw from PRs such as #38883 and #38671
Or maybe it's just too fuzzy an area to pin down. 🌪️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes absolutely. This should be a living document, like all our docs. I think we're still figuring a lot of that out, so it's hard to pen down right now. If you have any suggestions on things we can add now then please go ahead!
lib
folder for PHP code
Thanks for editing @ramonjd! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for starting this @noisysocks! I've just added a couple questions and a few tiny grammar suggestions, but this looks like a great start for a living document. Nice work!
// lib/experimental/navigation/class-wp-navigation.php | ||
|
||
if ( class_exists( 'WP_Navigation' ) ) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, TIL you can call return at root level in a PHP file (i.e. not inside a function) to skip execution 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP is pretty lawless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up! It's clear and detailed and will make everyone's life easier 🎉
|
||
### Note how your feature should look when merged to Core | ||
|
||
Developers should write a brief note about how their feature should be merged to Core, for example, which Core file or function should be patched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will help so much with backports to Core ⭐
Thanks for the proofreading and suggestions! I'll link to this in the #core-editor chat for awareness. Again, let's iterate on this document. The team is still figuring out a lot of it 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool addition, thanks Robert.
function gutenberg_get_navigation( $slug ) { ... } | ||
``` | ||
|
||
### Group PHP code by _feature_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think the lib/compat
folder should group its code by "core" php file. Meaning if the change needs to happen in edit-form-blocks.php
in 5.9, I'd put it in lib/compat/5.9/edit-form-blocks.php
and for new files, try to come up with the best file name that will suite core merge. In practice, this is also very close to "group by feature".
The main goal is to simplify the work for the person back porting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of that though is that you can't move features in and out of "experimental" status e.g. using git mv lib/experimental/cool-new-feature.php lib/compat/wordpress-6.0/cool-new-feature.php
. Not sure if we do this in practice, mind you 😀
### Prefer the `wp` prefix | ||
|
||
For features that may be merged to Core, it's best to use a `wp_` prefix for functions or a `WP_` prefix for classes. | ||
|
||
This applies to both experimental and stable features. | ||
|
||
Using the `wp_` prefix avoids us having to rename functions and classes from `gutenberg_` to `wp_` if the feature is merged to Core. | ||
|
||
Functions that are intended solely for the plugin, e.g., plugin infrastructure, should use the `gutenberg_` prefix. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on naming conventions for when we're updating a function from core?
For example, this change from https://github.com/WordPress/gutenberg/pull/38681/files?w=1#diff-089523f9c993d53f6f02926d28f7a80394d93740e7d16e47e17244547b2b8f5e. The core action is removed and the plugin action is added to replace it.
My intention with that change was that the gutenberg_
version of the function in the plugin would replace the wp_
version when porting to core. If we're reserving that prefix for things that should never go into core, we need another convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we really have a good strategy for this kind of stuff. We have a similar nightmare with WP_Theme_JSON
needing Gutenberg_Theme_JSON
and WP_Theme_JSON_5_9
😅
Using gutenberg_
seems fine to me, though it will get tricky when the new gutenberg_
function makes it into WP 6.0 and we want to make more changes in 6.1 We won't be able to tell whether the wp_
function that exists is the 5.9 version or the 6.0 version.
…ib` folder for PHP code (WordPress#39603) * Add lib/README.md * Subediting * Formatting * Fix typo Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Clarify why we want the wp_ prefix * Reword sentence about hooking * Fix typo * Clarify that gutenberg_ is acceptable for infrastrucutre Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Thanks indeed! These changes helped me better understand the methodology and nuances of the PHP merging process! |
What?
Adds some documentation about our PHP file structure and some best practices when writing PHP.
Why?
I want to make @tellthemachines happy.
How?
I wrote it and have not proofread it at all. Please feel free to push changes. It's based off of the discussion in #33810 and my own gut feelings having merged PHP code from Gutenberg to Core a few times.
Testing Instructions
Read the file.