Should we keep the download limit and download expiration change?

[rodrigoprimo]

WC core lets users set download limit and download expiration per product. When we designed the custom product tables schema, we decided to change that and let users set download limit and download expiration per file. I think that this is a nice feature to add to WC, but I'm not sure if we should have added it to this plugin before adding it to WC core.

The main problem with supporting download limit and download expiration per file in this plugin and not in WC core is that they can't share the same product API. For example, what should $product->get_download_limit() return if each product file can now have a different download limit? IMHO, if we keep this change, it will force us to keep two different versions of all the code that touches product downloads. It will also make it harder for anyone that wants to use this plugin (they will have to update code that uses product downloads and make sure that the extensions that they are using are compatible with this plugin).

There are four WC core unit tests that are failing because of the download limit and download expiration change:

• WC_Tests_Product_CSV_Importer::test_import() (see #78)
• WC_Tests_Customer_Functions::test_wc_get_customer_available_downloads() (see #77)
• WC_Tests_Customer_Functions::test_wc_update_new_customer_past_orders() (see #76)
• WC_Tests_Product_Data::test_product_getters_and_setters() (see #80)

Those failing tests illustrate some of the challenges that we will have to address if we decide to keep download limit and download expiration per file.

I suggest that we remove this change from the plugin for now and reintroduce it only after it is introduced to WC core (probably only in WC 4.0 as it is a breaking change). @kloon mentioned that the original idea when this was added to the plugin was to add it first to WC core anyway.

I'm opening this issue to ask for input from others and to continue the conversation we started earlier today before we make a decision. @claudiosanches mentioned that he thinks we should simply hide the new interface in the product edit page and keep the new data structure. Claudio, could you please elaborate on how that would work and why do you prefer this approach over waiting until WC core implements this feature to add it to this plugin?

[kloon]

Been thinking about this and agree we should rather add this to core first. On that I have thought about the blocker namely $product->get_download_limit() and what if we return an array of id=>value pairs if the product has different limits for each individual file.

Yes there is still potential to break, but ultimately a good developed plugin should be checking for an integer anyway, and with this change can just introduce an array check?

[claudiosanches]

My idea it's simple, we don't include that new interface anymore and we should handle the posted data almost like before, we get all _download_limit and _download_expiry from $_POST in admin and include it into wc_product_downloads database instead of use metadata or include new fields for it into wc_products database.

This is very easy to handle, we can keep using set_download_limit and set_download_expiry to set it for all downloads at the same time.

In summary: we'll only keep the wc_product_downloads structure, but all things will be like before.

And why? At least having the database set for all downloads, should be easy to migrate in the future. But do what you folks think that is best for it, I don't want to fight to keep this, or say where is the best place to break compatibility. But compatibility will break unless we never implement this feature.

[rodrigoprimo]

Thanks for your input, @kloon and @claudiosanches. Claudio, thanks as well for clarifying your idea.

Balancing avoiding breaking changes and easiness to implement given the advanced stage of this plugin development, I'm inclined to agree that simply removing the interface to set download limit and download expiration per file is the way to go. I just have two remaining concerns:

• Not sure if anyone uses it, but WC core currently supports setting a download limit and expiration to products without any files. In the new data structure added to this plugin, this is not possible. WC core tests use this to test product getters and setters (see test WC_Tests_Product_Data::test_product_getters_and_setters()). We will need to find a way to skip the part of the test that uses download limit and download expiration.

• This change relies on a few modifications to the WC core class WC_Product_Download. It adds new properties and methods to this class (see commits https://github.com/woocommerce/woocommerce/commit/5ef6766654159658c828d124bdc5623a0e3f0097, https://github.com/woocommerce/woocommerce/commit/18600e1d2fbca4b3e5fcb7c4250d16791b5cbc28, and https://github.com/woocommerce/woocommerce/commit/bdda142d248911b163069a39b484ad82a60f5ef6). Those changes will be visible to everyone, but they only make sense for those using this plugin. Calling WC_Product_Download::get_limit() when this plugin is not being used will return nothing and might mislead developers.

Thoughts? Maybe I'm worrying too much about issues that might happen only to a limit number of users?

[rodrigoprimo]

As I'm still unsure about this and worried that keeping this change might present some issues that we still have considered besides the two issues that I mentioned above, I created #91 which removes download limit and download expiry per file and creates two new fields to store this information in the wp_wc_products table. I decided to do this to see how complex it would be to implement this change and it was pretty straightforward. In my opinion, merging #91 is the best option we have now. We can revisit this idea later (I suggest after this change is done in core), but I don't oppose to closing the PR without merging it in case we decide to keep download limit and download expiry per file.

[rodrigoprimo]

See https://github.com/woocommerce/woocommerce-product-tables-feature-plugin/pull/91#issuecomment-384764927