search

humanmade / asset-manager-framework

A framework for overriding the WordPress media library with an external asset provider, such as a DAM
GNU General Public License v2.0
155 stars 7 forks source link

Multi-Provider OOP #29

Closed tfrommen closed 3 years ago

tfrommen commented 3 years ago

This is a first draft of my follow-up of Rob's extension of Shady's implementation of multiple providers. 😂

See #28.

tfrommen commented 3 years ago

I agree with not liking the singleton pattern, but given that the consumer code is a bunch of functions, I didn't see a good alternative.

Ideally, we would have separate classes that handle the AJAX callbacks, and these classes would have access to the registry via dependency injection. But changing all this felt way out of scope, so I decided for singleton—for now.

tfrommen commented 3 years ago

I don't think we need a function version for registration as you would, or rather: should only do that where you have access to the registry object anyway.

I can add one for get_provider, yeah, but there are only two usages. Do you think any third-party code needs to fetch a specific provider? If you want to decorate, you use the filter. If you want to get all, you can hook into the new action, and ask the registry. 🤔

roborourke commented 3 years ago

Yeah agreed, lets leave any wrapper functions out for now. I think this is good 👍

svandragt commented 3 years ago

Just wanted to highlight: can we make sure to follow semantic versioning on this breaking change? this has been a problem with previous breaking updates for my project. Thanks for the great work :)

tfrommen commented 3 years ago

@svandragt since this is a non-stable 0.x version, we are pretty much good to do whatever we want, really. Of course, this change should be anything but a patch release, so I am thinking 0.11.0. Given that there are already other bigger changes planned, we maybe shouldn't release this as 1.0.0. Not because of not wanting to do more major releases, but because there are things missing (upload/manipulation) that have been planned to be included right from the start.

All that said, if you are using Composer to pull this in, you current version constraint should be something like ^0.10. If we introduce this change and relase it as 0.11.0, Composer knows this is a potentially breaking change, simply because of the non-stable nature. So you wouldn't get version 0.11.0, even though you are using the caret , ^, operator. This is different from all stable releases, after 1.0.0, where the caret means patch and minor. For this, you will only ever receive patch updates.

Does that help?

roborourke commented 3 years ago

TIL about 0.x releases and composer 🤯