Skip to content
This repository has been archived by the owner on Oct 12, 2024. It is now read-only.

Package the plugin for developer use #91

Closed
wants to merge 21 commits into from

Conversation

citelao
Copy link

@citelao citelao commented Jul 19, 2017

Do not merge; opened for discussion

Goal

Bring the Fields API into production by making it easy to write plugins that use it; replacement of core fields can come later.

Proposal

The best solution seems to be making this a Composer plugin, intended to be bundled with a plugin that uses it, installed as its own plugin, or installed as a must-use plugin.

It is possible, albeit confusing, to support all of these at once, to some degree. The biggest problem is dependency hell, which we can mitigate. WP-SCP Framework and CMB2 are both written to automatically include only the latest version if multiple copies are installed; with a relatively simple addition we can also generate warning messages admin-side if conflicting versions exist.

Practically, we would:

  1. At plugin load-time, queue all copies of this plugin, no matter where they are located
  2. After plugins load, load the most recent version into its global, firing form-registration hooks etc
  3. If more than one version is present, display a warning on the admin screen

This change will also make replacing core forms optional, since that is the most brittle and experimental part of this project.

Pitfalls

There are some potential downsides, though, hence the discussion pull request:

  • This is not how the REST API was implemented; their bootstrap process was rudimentary, "first to load" (if( ! class_exists() ))
  • This API is not stable. Breaking changes to the API will break plugins that depend on earlier versions of the API, even if they include their own copies of it, since we load the most recent version no matter what. Our implementation can warn about this, but not prevent it.

The relevant discussions I saw, like dependency hell and WP core dev unhappiness (also see Justin Sternberg's musings about this and an issue on including frameworks that he linked), though all over a year old, indicate that including frameworks (like this) in plugin code is frowned upon. However, this "framework" is plotting for core inclusion. As far as I can tell, we can craft a bootstrap process that behaves nicely and defers to any eventual core implementation.

Alternatives

  • We could package solely as a WP plugin; developers could check for our code (is_plugin_active or something) and warn if our plugin is not installed

Final thoughts

Unfortunately, this is the best suggestion I could come up with; I spent a while thinking about implementation and I'm pretty sure we can have a nice UX for dependency hell.

I'd love any thoughts on this; any suggestions?

Action items

  • Encapsulate plugin bootstrap into namespace-ing class.
  • Decide on version-update mechanism and write appropriate tests
  • Transition unnecessary defines to class/instance variables
  • Make default (wp-admin/) form replacement optional
  • Localize strings 😲
  • TODO below

@sc0ttkclark
Copy link
Owner

Reviewing your PR here this week, thanks for all of your love and effort on this!

@sc0ttkclark
Copy link
Owner

PR reviewed, I meant reviewing the text of your PR here :)

@sc0ttkclark sc0ttkclark self-requested a review July 24, 2017 16:08
@sc0ttkclark sc0ttkclark self-assigned this Jul 24, 2017
@citelao
Copy link
Author

citelao commented Jul 26, 2017

@sc0ttkclark I'm a bit unclear---do you think my suggestion is the way to go, or will you be looking into it?

As an aside, I'm having a bit of a difficult time figuring out how to test my "inclusion" code. Suggestions welcome.

@sc0ttkclark
Copy link
Owner

sc0ttkclark commented Jul 26, 2017

Oh, I'm going to set aside some time this week to read your suggestion here in depth is what I was saying. Will get back to you on the test.

@sc0ttkclark
Copy link
Owner

Looks pretty good, I think I'd like to work with you on the initial class, there's a lot we need to pair down in there and restart somewhat.

@citelao
Copy link
Author

citelao commented Jul 28, 2017

Plugin bootstrap should now be all encapsulated in a class. If we commit to decrementing WP_Fields_API_v_{x}::$PRIORITY on each release, a la CMB2, we should have a working include system. I think we could do better (something with version_compare), but it's not strictly necessary.

Action items added to pull request description.

@jtsternberg
Copy link

Re: CMB2 loader, this tool makes it easy to generate a similar loader: http://jtsternberg.github.io/wp-lib-loader/

@citelao
Copy link
Author

citelao commented Jul 31, 2017

Per discussion in #core-fields Slack, next steps:

TODO in description and:

  • Bring up to par with @jtsternberg's WP-Lib-Loader
  • Add define based enabling/disabling of core overrides (with is_plugin_active check)
  • Make sure we still provide sensible (and localizable) warnings when multiple versions are installed.

With that done, this should be a robust (if highly experimental) plugin for potential implementation testing.

@citelao
Copy link
Author

citelao commented Nov 11, 2017

I took some time to think about this again, and I think I'm stuck with a couple of problems:

  • I want version updating from our side to be very easy, even based on a semantic version number rather than an iteration count if possible
  • I want to have a useful warning on the admin page if multiple versions are installed
  • I want to have a robust test harness for this new functionality, and I'm struggling to find a way to test inclusion code properly. For example, how do you test "multiple plugins including different versions" if a given implementation is only one version?

I figured it was time to reach back out to the community, instead of worrying about it and not being productive in the process 😄 . I'd like to help however I can, but this is definitely an area of testing I'm a bit lost in.

Thoughts?

@citelao citelao mentioned this pull request Nov 11, 2017
@citelao
Copy link
Author

citelao commented Dec 4, 2017

Any thoughts on this?

@sc0ttkclark
Copy link
Owner

I would hold on this until our next dev meeting, I'm waiting to get the details finalized on that.

@sc0ttkclark sc0ttkclark closed this Aug 2, 2023
@sc0ttkclark sc0ttkclark added the archived Issues from an archived version of the Fields API label Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Issues from an archived version of the Fields API question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants