We did a bit of thinking on how to re-architect the code. Here's what we came up with.

Issues

• interface Syndication_Client requires too much functionality from clients
  • Pull and Push functionality
  • Option saving/loading/validation
  • Option page rendering
  • In some cases, creating attachments and terms (even though WP_Push_Syndication_Server creates the actual posts)
    Because it’s an interface, the implementations are responsible for some behavior that must be consistent across all implementations, such as the public filters and actions used by other plugins to interface with this plugin. Right now this is done inconsistently so the hooks/filters behave differently depending on the client.
• Clients extend API client objects which creates confusion that makes it hard to update . understand
  • Syndication_WP_XMLRPC_Client extends WP_HTTP_IXR_Client, and Syndication_WP_RSS_Client extends SimplePie. This creates a hard dependency on objects that we may want to change in the future. It is also extremely confusing, because objects that already do too many things inherit all the behaviors of objects that do completely different things.

Proposed Solution

• Break the Syndication_Client interface into more targeted interfaces
  • Pull_Client_Interface
    • Responsible for pulling content from a remote source.
    • Pull clients will no longer be responsible for adding content to WP. It will simply pass an array of post, attachment, and term objects to the Pull_Manager.
  • Push_Client_Interface
    • Responsible for pushing content to a remote source.
    • Push clients will no longer be responsible for selecting content from the database, this will be the responsibility of the Push_Manager, which will pass an array of data to the Push_Client.
  • Client_Options_Interface
    • Responsible for loading, saving, and validating client options.
  • Client_Options_Page_Interface
    • Responsible for rendering and saving the options form
• Create Push_Manager object
  • Responsible for preparing content for a Push_Client
  • Applies public filters and actions to data before passing it to a Push_Client
• Create a Pull_Manager object
  • Responsible for handling content from a Pull_Client
  • Applies public filters and actions to data returned by a Pull_Client
  • Also solves issue #47 because there is one object responsible for adding data to WP, so it can also make sure object cache issues are handled properly
• Separate clients from the API client objects they use
  • Use composition to tie Pull_Clients and Push_Clients to the APIs they need to do their job. E.g. SimplePie instance as a member of a Pull_Client rather than extending it. It is far less confusing to have instances of these objects as members on the client.

Possible issues

• Backward compatibility
  • When restructuring the client do we need to keep backward compatibility in mind. Other people might have written extensions which rely on current plugin structure.
• Developer awareness
  • As we're moving to a more OOP based approach, can we assume that the target audience of this plugin will be able to understand the code and work with i?

It would be great if you could give us some feedback and let us know if this makes sense and if you'd like us to move ahead with this or give us some pointers if you want to steer this in an other direction

from syndication.

I think the issues you've identified are spot on.

I'm wondering if the Pull_Client_Interface and Pull_Manager can be combined as an abstract class. Similarly Push_Client_Interface and Push_Manager. Do you have any thoughts about why it's better to keep them separate?

For options, I think we want to have largely the same options across every syndication method. Obviously there will be some minimal differences related to authentication. Maybe instead of building our own options interface, we can have a standard set of options and add additional fields with the WordPress settings API?

My goal is to reduce complexity and take advantage of as much of the WordPress APIs as possible. Some level of inheritance will be necessary to reduce repetition, but a big inheritance tree makes the code harder to grok for anyone looking at it for the first time.

When restructuring the client do we need to keep backward compatibility in mind. Other people might have written extensions which rely on current plugin structure.

If we can keep backward compatibility for actions and filters, I think that will be enough.

As we're moving to a more OOP based approach, can we assume that the target audience of this plugin will be able to understand the code and work with i?

I'm willing to assume the target audience understands OOP. As noted, I agree that large inheritance trees can be confusing, so keeping that to a minimum while not repeating ourselves is ideal.

from syndication.

I'm wondering if the Pull_Client_Interface and Pull_Manager can be combined as an abstract class. Similarly Push_Client_Interface and Push_Manager. Do you have any thoughts about why it's better to keep them separate?

They are separate because there are operations that are client-specific and operations that are the same for all clients. For instance, Pull_Manager would handle inserting the posts into the database, while a Pull_Client_Interface implementation does the work necessary to fetch those posts. Combining them into a single abstract class puts the burden of inserting the posts on the client, which will lead to inconsistent implementations--a problem in the current plugin that we are trying to fix.

(Quick edit: this is also important to make sure action and filter hooks are consistent across all client implementations.)

Maybe instead of building our own options interface, we can have a standard set of options and add additional fields with the WordPress settings API?

We do intend to use the Settings API to register and validate the options. These interfaces are only to provide a consistent way for the plugin to find and trigger these operations for the specific clients. Also, the plugin may need to trigger the rendering of client-specific options since these are displayed on the individual site editing pages, not standard option pages.

The settings API will be used entirely for plugin-level options.

Some level of inheritance will be necessary to reduce repetition, but a big inheritance tree makes the code harder to grok for anyone looking at it for the first time.

Our goal is to keep things as flat as possible, while defining a consistent API for the clients (and future client developers) to implement. As planned there should never be a class that extends another class, just some classes that implement interfaces.

If we can keep backward compatibility for actions and filters, I think that will be enough.

We will write tests to try and keep these actions and filters as compatible as possible.

from syndication.

For instance, Pull_Manager would handle inserting the posts into the database, while a Pull_Client_Interface implementation does the work necessary to fetch those posts.

Why can't Pull_Client be an abstract class that both inserts posts into the database and defines abstract functions that do the work necessary to fetch posts?

Also, the plugin may need to trigger the rendering of client-specific options since these are displayed on the individual site editing pages, not standard option pages.

My thinking was that we could define the standard options page and any syndication methods that need to add more fields could use the Settings API to add fields to that page. I think the client-specific options will be pretty limited -- maybe just authentication in most cases. I'm not sure the extra complexity of two extra classes is worth it if we just need to add a username and password to some clients.

from syndication.

Why can't Pull_Client be an abstract class that both inserts posts into the database and defines abstract functions that do the work necessary to fetch posts?

That should work too. I think this is just a matter of preference: I prefer small classes with small goals. We probably won’t know how some of this shakes out until we start writing code, but I am happy to steer towards your preferences on issues like this.

My thinking was that we could define the standard options page and any syndication methods that need to add more fields could use the Settings API to add fields to that page. I think the client-specific options will be pretty limited -- maybe just authentication in most cases. I'm not sure the extra complexity of two extra classes is worth it if we just need to add a username and password to some clients.

Sorry, I’ve made this confusing because I am talking about two different things at once: the plugin settings (which go in blog options) and the site-specific settings (currently stored as meta on individual syn_site posts).

For the site-specific settings (and some clients, like XML Pull, have many), the settings API won’t really work because they’re not stored in blog options. That’s why I needed a way for clients to know it was time to render or save their settings. On second thought I realize using interfaces for this is silly: we can just do_action() to let clients know it’s time to render or save. That completely removes the need for interfaces and gives the freedom to render or save however works best for each client.

from syndication.

That sounds good. Can you give a brief overview of the revised plan for the
architecture just to make sure I'm clear on how this will work?

On Thu, Mar 12, 2015 at 9:05 AM, Chancey Mathews [email protected]
wrote:

Why can't Pull_Client be an abstract class that both inserts posts into
the database and defines abstract functions that do the work necessary to
fetch posts?
That should work too. I think this is just a matter of preference: I
prefer small classes with small goals. We probably won’t know how some of
this shakes out until we start writing code, but I am happy to steer
towards your preferences on issues like this.

My thinking was that we could define the standard options page and any
syndication methods that need to add more fields could use the Settings API
to add fields to that page. I think the client-specific options will be
pretty limited -- maybe just authentication in most cases. I'm not sure the
extra complexity of two extra classes is worth it if we just need to add a
username and password to some clients.

Sorry, I’ve made this confusing because I am talking about two different
things at once: the plugin settings (which go in blog options) and the
site-specific settings (currently stored as meta on individual syn_site
posts).

For the site-specific settings (and some clients, like XML Pull, have
many), the settings API won’t really work because they’re not stored in
blog options. That’s why I needed a way for clients to know it was time to
render or save their settings. On second thought I realize using interfaces
for this is silly: we can just do_action() to let clients know it’s time to
render or save. That completely removes the need for interfaces and gives
the freedom to render or save however works best for each client.

—
Reply to this email directly or view it on GitHub
#46 (comment)
.

from syndication.

Should look something like this.

• abstract Pull_Client
  • abstract methods
    • Responsible for pulling content from a remote source.
    • Abstract methods are not responsible for adding content to WP. They will simply return an array of post, attachment, and term objects to be handled by the class’s common methods.
  • common methods
    • Responsible for handling content from the abstract methods.
    • Applies public filters and actions.
    • Also solves issue #47 because there is one object responsible for adding data to WP, so it can also make sure object cache issues are handled properly.
• abstract Push_Client
  • abstract methods
    • Responsible for pushing content to a remote source.
    • Abstract methods are not responsible for selecting content from the database, this will be the responsibility of the class’s common methods.
  • common methods
    • Responsible for preparing content for a Push_Client
    • Applies public filters and actions to data before passing it to a Push_Client
• class Plugin
  • Responsible for loading up the above objects and wiring them together.

from syndication.

👍 Sounds awesome.

from syndication.

Great! Can you confirm that we can go ahead and start working on this?

from syndication.

Yep.

from syndication.

Awesome. We'll get moving on this.

from syndication.

Let's finally get this closed out.

• Remove types -- there are already objects that represent these in WordPress
• Centralize custom-post-types, custom-taxonomies, and class-cron.php into push-syndication.php
• Remove duplicate code in class-walker-category-dropdown-multiple.php
• Remove push/pull interfaces

Clients:

• RSS Client
• XML Client
• XML-RPC Client
• REST Client

@tott I had mentioned removing the auto loader, but I think you're right that it makes creating clients easier. If we can get these things done, I think we can close out this issue. Let me know what you think.

from syndication.

These are all completed in my latest mater branch, except 'Remove types -- there are already objects that represent these in WordPress' - I think we were discussing this further.

https://github.com/adamsilverstein/syndication/commits/master

from syndication.