Update/oop all the things #273

darylldoyle · 2025-01-30T17:34:00Z

Description of the Change

This PR moves the wp-scaffold to use a OOP approach that more closely aligns with industry standards. Currently we have classes containing actions/filters, but we also have functional PHP files containing actions/filters. The issue with mixing them is that:

We end up with poorly organised code because engineers disagree about where the best place to put them is.
Mixing functional and OOP code can be confusing when we could use OOP for everything.
Having one or the other would make it easier to implement tests across projects/the scaffold as we'd need less manual loading.

Closes #251, #247

How to test the Change

Ensure everything still works as expected

Changelog Entry

Developer - Moved to a more OOP structure.

Credits

Props @darylldoyle, @tobeycodes

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added Critical Flows, Test Cases, and/or End-to-End Tests to cover my change.
All new and existing tests pass.

tobeycodes

Looks great!

tobeycodes · 2025-01-30T18:14:32Z

mu-plugins/10up-plugin/plugin.php

At the top of this file we should throw an error if \TenUpPlugin\PluginCore does not exist telling the developer to do a composer install

mu-plugins/10up-plugin/plugin.php

tobeycodes · 2025-01-30T18:15:32Z

mu-plugins/10up-plugin/src/Assets.php

+	 *
+	 * @return bool
+	 */
+	public function can_register() {


Could this be the default?

That change would need to be made in the framework, but I don't think it's a bad idea 🤔

tobeycodes · 2025-01-30T18:16:41Z

mu-plugins/10up-plugin/src/Assets.php

+	 * @param string $handle The script handle.
+	 * @return string|null
+	 */
+	public function script_loader_tag( $tag, $handle ) {


Wasn't this supported a while ago? https://make.wordpress.org/core/2023/07/14/registering-scripts-with-async-and-defer-attributes-in-wordpress-6-3/

I really hope we can remove this as well.

Agreed, I don't think this is the right place to do that though. This is primarily around changing the structure of the scaffold, so I'm trying to move things 1:1. Let's spin up an issue to do this though!

tobeycodes · 2025-01-30T18:17:12Z

mu-plugins/10up-plugin/src/Assets.php

+	 * @param string $stylesheets Comma-delimited list of stylesheets.
+	 * @return string
+	 */
+	public function mce_css( $stylesheets ) {


Is this still needed? When do projects not use guteneberg?

Please please please remove this code. I don't know a single person who uses this functionality.

As above, let's spin up an issue to remove this. This PR is about the structure

tobeycodes · 2025-01-30T18:18:19Z

mu-plugins/10up-plugin/src/Assets.php

+	 *
+	 * @return void
+	 */
+	public function register() {


I would love to see us automate enqueueing scripts/styles. we can find all the scripts/styles in the dist folder and should be able to infer their context (or add it easily)

This would be an amazing enhancement. CC @fabiankaegy Toby gave me some thoughts on this in Slack too.

tobeycodes · 2025-01-30T18:19:41Z

mu-plugins/10up-plugin/src/PluginCore.php

I'd move this to the framework. For the variables such as tenup-plugin move these to constants the theme controls. The plugin itself can extend the class and override things when needed e.g. register

I'm not against this as an enhancement, but I'd love to understand your thinking around overrides etc.

claytoncollie · 2025-01-31T10:34:38Z

mu-plugins/10up-plugin/src/Core/Emoji.php

 */
-class Overrides implements ModuleInterface {
+class Emoji implements ModuleInterface {


Can we move this to the framework and register it by default?

Can you open that as an issue on the wp-framework repo, please? I think that's a great idea, but this PR isn't the right place.

claytoncollie · 2025-01-31T10:35:11Z

mu-plugins/10up-plugin/src/Core/HeadOverrides.php

+ *
+ * @package TenUpPlugin\Core
+ */
+class HeadOverrides implements ModuleInterface {


Can we move this to the framework?

As above, can you open that as an issue on the wp-framework repo, please? I think that's a great idea, but this PR isn't the right place.

fabiankaegy

Left a few small questions / comments but overall looking forward to the new stricture :)

Also lets make sure we add the one place for utilities as discussed in the internal projects call 👍

fabiankaegy · 2025-02-17T07:34:21Z

themes/10up-block-theme/package.json

+		},
+    "paths": {
+      "blocksDir": "./blocks"
+    }


Nit: It seems the indentation here is of

fabiankaegy · 2025-02-17T07:36:12Z

themes/10up-block-theme/src/Assets.php

+	/**
+	 * Can this module be registered?
+	 *
+	 * @return bool
+	 */
+	public function can_register() {
+		return true;
+	}


Stupid question:

Would it not be better to have this return true by default in the ModuleInterface base class? In 90% of cases that is what we want. And when we don't we can override it. But it seems redundant to always have to add this to every single file 🤔 (very loosely held opinion)

I agree, though that's a change we'll need to make in the Framework and then update here 🙂

themes/10up-block-theme/src/Assets.php

fabiankaegy · 2025-02-17T07:38:24Z

themes/10up-block-theme/src/Assets.php

+	 * @return void
+	 */
+	public function scripts() {
+


nit: to stay consistent lets remove this empty line

fabiankaegy · 2025-02-17T07:41:26Z

themes/10up-block-theme/src/Blocks.php

+ */
+class Blocks implements ModuleInterface {
+
+	use Module;


Stupid question:

Why do we need to add use Module here? Isn't that already coming from the implements ModuleInterface?

Not a stupid question! The ModuleInterface tells the class what methods it needs to implement, the Module trait does the actual implementation.

The Interface/Trait approach do the same thing as the old Module abstract class used to, without the downside of everything having to inherit a single class. This makes it more flexible for engineers to change those methods when they have things they actually want to do.

fabiankaegy · 2025-02-17T07:42:55Z

themes/10up-theme/package.json

+      "block-editor-script": "./assets/js/block-editor/block-editor-script.js"
+    },
+    "paths": {
+      "blocksDir": "./blocks"


nit: Indentation here is of

fabiankaegy · 2025-02-17T07:43:53Z

themes/10up-theme/src/Assets.php

+		/**
+		 * Enqueuing shared.js is required to get css hot reloading working in the frontend
+		 * If you're not shipping any shared js wrap this enqueue in a SCRIPT_DEBUG check.
+		 */
+
+		/*
+		 * Uncoment this to use the shared.js file.
+			wp_enqueue_script(
+				'shared',
+				TENUP_THEME_TEMPLATE_URL . '/dist/js/shared.js',
+				$this->get_asset_info( 'shared', 'dependencies' ),
+				$this->get_asset_info( 'shared', 'version' ),
+				true
+			);
+		*/


Doesn't need to be in this PR but I'm in favor of removing all the "shared" stuff here also...

fabiankaegy · 2025-02-17T07:44:13Z

themes/10up-theme/src/Assets.php

+	 * @return void
+	 */
+	public function admin_styles() {
+


nit: remove empty line

fabiankaegy · 2025-02-17T07:44:57Z

themes/10up-theme/src/Blocks.php

+		add_action( 'init', [ $this, 'register_theme_blocks' ] );
+		add_action( 'init', [ $this, 'register_block_pattern_categories' ] );
+	}
+


nit: remove additional empty line

darylldoyle added 4 commits January 30, 2025 15:04

Move everything into a src directory

be69e20

Split the functional code into a couple of classes and update the init

03bcfd8

Merge branch 'trunk' into update/oop-all-the-things

8aa0c03

Split overrides out into smaller classes

bdef91e

darylldoyle self-assigned this Jan 30, 2025

tobeycodes approved these changes Jan 30, 2025

View reviewed changes

claytoncollie reviewed Jan 31, 2025

View reviewed changes

darylldoyle added 9 commits February 6, 2025 14:25

Remove singleton

3b24643

lint fix

757fd2b

Merge branch 'trunk' into update/oop-all-the-things

2551d3e

Linting

eceaa18

Fix PHPCS

c6edff6

OOP the theme

c0673ed

Move get_asset_info into trait

4339a63

Oop the block theme

30ba5b3

Linting fix

47c7286

darylldoyle marked this pull request as ready for review February 6, 2025 16:54

darylldoyle requested review from fabiankaegy and tlovett1 February 6, 2025 16:54

fabiankaegy approved these changes Feb 17, 2025

View reviewed changes

darylldoyle added 6 commits February 17, 2025 10:28

Include name

01baf5e

PR feedback

27d971d

Add new template-tags.php file

1d4e80c

Add namespaces

14a07de

Fix linting issue

5ed65f0

Docs update

c7be1c8

darylldoyle merged commit 77d7aaf into trunk Feb 17, 2025
7 checks passed

darylldoyle deleted the update/oop-all-the-things branch February 17, 2025 11:19

darylldoyle mentioned this pull request Feb 17, 2025

Replace function files with module classes #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/oop all the things #273

Update/oop all the things #273

darylldoyle commented Jan 30, 2025

tobeycodes left a comment

tobeycodes Jan 30, 2025

tobeycodes Jan 30, 2025

darylldoyle Feb 6, 2025

tobeycodes Jan 30, 2025

claytoncollie Jan 31, 2025

darylldoyle Feb 6, 2025

tobeycodes Jan 30, 2025

claytoncollie Jan 31, 2025

darylldoyle Feb 6, 2025

tobeycodes Jan 30, 2025

darylldoyle Feb 6, 2025

tobeycodes Jan 30, 2025

darylldoyle Feb 6, 2025

claytoncollie Jan 31, 2025 •

edited

Loading

darylldoyle Feb 6, 2025

claytoncollie Jan 31, 2025

darylldoyle Feb 6, 2025

fabiankaegy left a comment

fabiankaegy Feb 17, 2025

fabiankaegy Feb 17, 2025

darylldoyle Feb 17, 2025

fabiankaegy Feb 17, 2025

fabiankaegy Feb 17, 2025

darylldoyle Feb 17, 2025

fabiankaegy Feb 17, 2025

fabiankaegy Feb 17, 2025

fabiankaegy Feb 17, 2025

fabiankaegy Feb 17, 2025

Update/oop all the things #273

Update/oop all the things #273

Conversation

darylldoyle commented Jan 30, 2025

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

tobeycodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claytoncollie Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiankaegy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claytoncollie Jan 31, 2025 •

edited

Loading