Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Subediting
  • Loading branch information
ramonjd authored Mar 21, 2022
commit abbfb8b8f146efb4ffd1e8bd4b84d88837ac00c3
92 changes: 41 additions & 51 deletions lib/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ release. Some features remain in the plugin forever or are removed.

To make it easier for contribtors to know which features need to be merged to
Core and which features can be deleted, Gutenberg uses the following file
strucutre for its PHP code:
structure for its PHP code:

- `lib/experimental` - Experimental features that exist only in the plugin. They
are not ready to be merged to Core.
Expand All @@ -27,99 +27,77 @@ 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.

This meakes it easier for contributors and plugin developers as there is less
cumbersome renaming of functions and classes from `gutenberg_` to `wp_` if the
feature is merged to Core.
This minimizes the cumbersome process of renaming 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 infastrucutre,
should use the `gutenberg_` prefix.
Functions that are intended solely for the plugin, e.g. plugin infrastructure, should use the `gutenberg_` prefix.

#### Bad
#### Good

```php
function gutenberg_get_navigation( $slug ) {
}
function wp_get_navigation( $slug ) {}
```

#### Good
#### Not so good

```php
function wp_get_navigation( $slug ) {
}
function gutenberg_get_navigation( $slug ) {}
```

### Group PHP code by _feature_
Copy link
Contributor

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.

Copy link
Member Author

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 😀


Developers should organize their PHP into files or folders by _feature_, not by
_component_.

Relatedly, developers should call `add_action` and `add_filter` immediately
Also, developers should call `add_action` and `add_filter` immediately
after the function being hooked is defined.

These two practices make it easier for PHP code to start in one folder (e.g.
`lib/experimental`) and eventually move to another using a simple `git mv`.

#### Bad
#### Good

```php
// lib/experimental/functions.php
// lib/experimental/navigation.php

function wp_get_navigation( $slug ) {
}
function wp_get_navigation( $slug ) { ... }

// lib/experimental/post-types.php

function wp_register_navigation_cpt() {
}
function wp_register_navigation_cpt() { ... }

// lib/experimental/init.php
add_action( 'init', 'wp_register_navigation_cpt' );
```

#### Good
#### Not so goo

```php
// lib/experimental/navigation.php
// lib/experimental/functions.php

function wp_get_navigation( $slug ) {
}
function wp_get_navigation( $slug ) { ... }

function wp_register_navigation_cpt() {
}
// lib/experimental/post-types.php

function wp_register_navigation_cpt() { ... }

// lib/experimental/init.php
add_action( 'init', 'wp_register_navigation_cpt' );
```

### Wrap functions and classes with `! function_exists` and `! class_exists`

Developers should take care to not define a function or class if it already
is defined.
Developers should take care to not define functions and classes that are already defined.

This ensures that, if the feature is merged to Core, that there are no fatal
errors caused by Core defining the symbol once and then Gutenberg defining it a
second time.
When writing new function and classes, it's good practice to use `! function_exists` and `! class_exists`.

#### Bad
If Core has defined a symbol once and then Gutenberg defines it a second time, fatal errors will occur.

```php
// lib/experimental/navigation/navigation.php

function wp_get_navigation( $slug ) {
}

// lib/experimental/navigation/class-gutenberg-navigation.php

class WP_Navigation {
}
```
Wrapping functions and classes avoids such errors if the feature is merged to Core.

#### Good

```php
// lib/experimental/navigation/navigation.php

if ( ! function_exists( 'wp_get_navigation' ) ) {
function wp_get_navigation( $slug ) {
}
function wp_get_navigation( $slug ) { ... }
}

// lib/experimental/navigation/class-wp-navigation.php
Expand All @@ -128,10 +106,23 @@ if ( class_exists( 'WP_Navigation' ) ) {
return;
Copy link
Contributor

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP is pretty lawless.

}

class WP_Navigation {
}
class WP_Navigation { ... }
```

#### Not so good

```php
// lib/experimental/navigation/navigation.php

function wp_get_navigation( $slug ) { ... }

// lib/experimental/navigation/class-gutenberg-navigation.php

class WP_Navigation { ... }
```

Furthermore, a quick codebase search will also help you know if your new method is unique.

### Note how your feature should look when merged to Core

Developers should write a brief note about how their feature should be merged to
Expand All @@ -153,6 +144,5 @@ into Core.
*
* @return WP_Navigation
*/
function wp_get_navigation( $slug ) {
}
function wp_get_navigation( $slug ) { ... }
```