awesomemotive / easy-digital-downloads

Sell digital downloads through WordPress
https://easydigitaldownloads.com
GNU General Public License v2.0
864 stars 475 forks source link

Introduce edd_item_supports() function #2955

Open pippinsplugins opened 9 years ago

pippinsplugins commented 9 years ago

We should introduce an edd_item_supports() function that would allow developers to determine if a product supports various features.

For example:

edd_item_supports( 'quantities' );
edd_item_supports( 'taxes' );
edd_item_supports( 'shipping' );

This should be introduced as a method in the EDD_Download class that then has a global wrapper function.

evertiro commented 9 years ago

oooh! I like that idea!

cklosowski commented 9 years ago

:+1:

danieliser commented 9 years ago

How would you implement it? I'm assuming in the Download class you would have a filter that developers could hook into to check meta and enable features on the fly storing them in a private class variable?

Sounds relatively straight forward. I can take this one.

pippinsplugins commented 9 years ago

The plan is to have it work just like add/remove_theme_support() and add/remove_post_type_supports().

PRs always welcome :)

danieliser commented 9 years ago

So the question becomes, since both of those are based on static items. Post Types & Themes aren't quite as dynamic as say download 242 supports feature x.

So this becomes a template function that uses the current query args similar to how get_the_content( $id = 0 ) works.

In which case I would just make functions to wrap new methods in EDD_Download class.

Unless I misunderstood the usage. But I can't quite see a general download_supports('key') functions usage.

But a download_support( $id, $key ) sounds like something I would use during templating or in a loop mainly.

If this is correct by your original idea then I can take this one as well.

pippinsplugins commented 9 years ago

We need to introduce both a method in EDD_Download and a global helper function that calls the method.

On Sun, Jan 18, 2015 at 7:19 PM, Daniel Iser notifications@github.com wrote:

So the question becomes, since both of those are based on static items. Post Types & Themes aren't quite as dynamic as say download 242 supports feature x.

So this becomes a template function that uses the current query args similar to how get_the_content( $id = 0 ) works.

In which case I would just make functions to wrap new methods in EDD_Download class.

Unless I misunderstood the usage. But I can't quite see a general download_supports('key') functions usage.

But a download_support( $id, $key ) sounds like something I would use during templating or in a loop mainly.

If this is correct by your original idea then I can take this one as well.

— Reply to this email directly or view it on GitHub https://github.com/easydigitaldownloads/Easy-Digital-Downloads/issues/2955#issuecomment-70436207 .

danieliser commented 9 years ago

OK, that's how I had it worked in my head. I can get this done. Just wanted to be sure it matched your original Idea, in that it is a function to check that each individual download supports a feature.

pippinsplugins commented 9 years ago

If you end up working on it, post a proof of concept before going all the way through it, just to ensure everyone is on the same page and work doesn't get wasted.

danieliser commented 9 years ago

No problem. What do you suggest? The functionality itself is rather simple. You want an outline of how it will be integrated and used? Can do that for sure.

pippinsplugins commented 9 years ago

Just a basic proposal of the API itself.

danieliser commented 9 years ago

Sure thing. WIll post it here.

pippinsplugins commented 9 years ago

I'm going to go ahead and punt this to 2.4 since it's still a nice to have.

danieliser commented 9 years ago

Will get this written up this week. That way it will be ready for testing early in the 2.4 cycle.

pippinsplugins commented 9 years ago

:+1:

On Tue, Feb 10, 2015 at 9:23 PM, Daniel Iser notifications@github.com wrote:

Will get this written up this week. That way it will be ready for testing early in the 2.4 cycle.

— Reply to this email directly or view it on GitHub https://github.com/easydigitaldownloads/Easy-Digital-Downloads/issues/2955#issuecomment-73828725 .

danieliser commented 9 years ago

So here is what I have worked out. Initially the way pippin proposed it, it was described to be on a per download basis, but the functions he said it would be similar to are not built the same way.

Referring to his mention of add/remove_theme_support() and add/remove_post_type_supports().

Both of those assume that the download is a global thing, not something that could have multiple instances.

I'm proposing that it be done via a method in EDD_Download just like he initially mentioned. This will include an array of supported feaures.

private $supports = array(
    'quantities' => false,
    'taxes'      => false,
    'shipping'   => false,
);

This will be passed through a filter edd_download_supports which will allow both the setting of each feature to true or false, as well as the addition of extra features from extensions.

Usage would be something along these lines.

apply_filter( 'edd_download_supports', 'edd_download_supports_taxes' );
function edd_download_supports_taxes( $supports, $download_id ) {
    if ( edd_item_quantities_enabled( $download_id ) ) {
        $supports['quantity'] = true;
    }
    return $supports;
}

If not in the loop you would use something like

edd_download_supports( 123, 'quantity' );

If in the loop then something like

edd_item_supports( 'quantity' );

I have the commits done locally, as long as usage is along the lines of how the core team wants it I will make a PR.

danieliser commented 9 years ago

I have also considered allowing an array to be passed to boeth edd_download_supports and edd_item_supports to check if multiple features are supported at once.

That way you could check if a download supports both shipping & taxes in one check. This doesn't have an immediately apparent use, but it may in the future allow checking that multiple items are true before showing/doing something.

Would use type checking so it shouldn't be too difficult to make this change and support passing a string or array as the $feature argument.

pippinsplugins commented 9 years ago

That's exactly what I'd like to see :+1:

On Saturday, February 21, 2015, Daniel Iser notifications@github.com wrote:

I have also considered allowing an array to be passed to boeth edd_download_supports and edd_item_supports to check if multiple features are supported at once.

That way you could check if a download supports both shipping & taxes in one check. This doesn't have an immediately apparent use, but it may in the future allow checking that multiple items are true before showing/doing something.

Would use type checking so it shouldn't be too difficult to make this change and support passing a string or array as the $feature argument.

— Reply to this email directly or view it on GitHub https://github.com/easydigitaldownloads/Easy-Digital-Downloads/issues/2955#issuecomment-75405527 .

danieliser commented 9 years ago

@Pippinsplugins perfect then. I have the commits done and will make a PR shortly. All that's missing is adding the built in features and checks to set the properly for each download. The setting and support check functions are all there already. It just needs to be integrated into existing systems and tested.

pippinsplugins commented 9 years ago

Excellent, that will be a great start.

On Sunday, February 22, 2015, Daniel Iser notifications@github.com wrote:

@Pippinsplugins https://github.com/Pippinsplugins perfect then. I have the commits done and will make a PR shortly. All that's missing is adding the built in features and checks to set the properly for each download. The setting and support check functions are all there already. It just needs to be integrated into existing systems and tested.

— Reply to this email directly or view it on GitHub https://github.com/easydigitaldownloads/Easy-Digital-Downloads/issues/2955#issuecomment-75450852 .

danieliser commented 9 years ago

PR submitted.

pippinsplugins commented 9 years ago

I've merged the WIP code eom @danieliser into issue/2955

pippinsplugins commented 9 years ago

Doh, turns out we can't use the filter name of edd_download_supports as we already use that to determine which features the download post type supports.

pippinsplugins commented 9 years ago

I don't love it but for now I"m going to propose edd_does_download_support.

pippinsplugins commented 9 years ago

I'm not sure we should be marking taxes as a feature here. We already support edd_download_is_tax_exclusive() via a checkbox in the UI. Should we have two filters that can affect this?

pippinsplugins commented 9 years ago

I'm also wondering if we should move some of our existing features to this item supports API, such as variable prices, file downloads, buy now buttons, etc.

Should we go 100% or just leave it to select features?

cklosowski commented 8 years ago

Going to punt for now. As discussed in the team meeting, we need to get some details worked out about how this will work with backwards compatibility.

danieliser commented 8 years ago

@pippinsplugins - I think it makes since that nearly all core stuff should be run through it at some point in the future. Though you may plan it over several release cycles.