Skip to content
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

feature: add channel registry #251

Closed
wants to merge 18 commits into from

Conversation

johnhooks
Copy link
Collaborator

@johnhooks johnhooks commented Apr 10, 2023

What?

Add a Channel_Registry class to register notification channels.

Why?

To attempt registering channels in code.

Implementing the data model of PR #168 has the challenge of when and how to look up plugin registered channels. If channel data is stored in a database table and an integer id column is used to link a channel to subscription and messages, how does a plugin know the id of the channel it registered? This data would have to be looked up on message emit, and also there wouldn't be a good mechanism to prevent plugins from registering similarly named channels.

Using a registry similar block types might work well. The channel is registered in code and the channel name/key/slug (as in core/post-edit) would link a message source to its channel. Subscriptions could also use the same key to link users to channels. This would also allow checking for conflicts at channel registration.

Keeping in mind that if/when a plugin is uninstalled, a message has to contain all the data required to render, it is necessary to store channel key and title in the message.

Both systems have pros and cons. This strategy removes the need for a channel table, and simplifies the logic of emitting a message. Though it will result in duplication of data in the database.

How?

  • Channel_Registry class, heavy based on core Block_Type_Registry
  • Channel class
  • register_channel function

Further considerations

  • When/if a plugin is uninstalled its channel subscriptions would remain in the database subscriptions table. Though this is a very small table compared with the queue table. And remembering user preferences is probably normal behavior, so there is a seamless experience if a plugin is reinstalled.
  • The channel icon could be stored in the message's icon column if/when a message doesn't specify it's own.

@johnhooks johnhooks self-assigned this Apr 10, 2023
@johnhooks johnhooks added [Scope] Model Definition of the data used in the WP Notify project [Scope] Service The core logic of the WP Notify project. php Pull requests that update Php code labels Apr 10, 2023
includes/channels.php Outdated Show resolved Hide resolved
@johnhooks
Copy link
Collaborator Author

johnhooks commented Apr 10, 2023

The register_channel function seems awkward. It could look cleaner to pass an array with channel constructor parameters.

Though this achieves creating a channel with translations for title and description.

register_channel(
new Notifications\Channel(
'core/updates',
__( 'WordPress Updates', 'wp-feature-notifications' ),
'WordPress', // TODO the linter won't leave a lowercased name for Dashicons.
__( 'WordPress core update events.', 'wp-feature-notifications' ),
)
);

@johnhooks johnhooks force-pushed the feature/add-core-classes branch from a33543c to 14dfc8e Compare April 10, 2023 10:59
@johnhooks johnhooks marked this pull request as draft April 10, 2023 10:59
@johnhooks
Copy link
Collaborator Author

This looks a lot cleaner to me, though PHP doesn't help with type hinting the array. Named arguments would be most preferred, but that would require a minimum PHP version of 8.

register_channel(
new Notifications\Channel(
'core/updates',
array(
'title' => __( 'WordPress Updates', 'wp-feature-notifications' ),
'icon' => 'wordpress',
'description' => __( 'WordPress core update events.', 'wp-feature-notifications' ),
)
)
);

@johnhooks johnhooks force-pushed the feature/add-core-classes branch from 6098a52 to e4f5897 Compare April 11, 2023 18:03
@johnhooks johnhooks marked this pull request as ready for review April 11, 2023 18:08
@johnhooks johnhooks requested review from erikyo and Sephsekla April 11, 2023 18:09
@johnhooks johnhooks force-pushed the feature/add-core-classes branch from 8ee8cb9 to a57fbd2 Compare April 11, 2023 21:24
*
* Instantiates a Channel object.
*
* @see WP\Notifications\register_channel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

wasn't something like this?
@see \WP\Notifications\register_channel()

@johnhooks johnhooks force-pushed the feature/add-core-classes branch from fe869eb to 9831729 Compare April 12, 2023 17:15
@johnhooks johnhooks added the [Status] Blocked When another issue need to be taken care of first. label Apr 12, 2023
@johnhooks
Copy link
Collaborator Author

Blocked by #255

@johnhooks johnhooks marked this pull request as draft April 12, 2023 19:47
@johnhooks johnhooks mentioned this pull request Apr 12, 2023
38 tasks
@johnhooks
Copy link
Collaborator Author

Work on this is being continued in #259

@johnhooks johnhooks closed this Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
php Pull requests that update Php code [Scope] Model Definition of the data used in the WP Notify project [Scope] Service The core logic of the WP Notify project. [Status] Blocked When another issue need to be taken care of first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants