New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.7] Some product functions moved to global scope not backwards compatible or meaningful #12495

Closed
franticpsyx opened this Issue Nov 30, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@franticpsyx
Member

franticpsyx commented Nov 30, 2016

As discussed in #12494 I see some product functions unrelated to CRUD that have been moved from product scope to global with no apparent reason or consistency with the rest.

A look at WC_Abstract_Legacy_Product in an attempt to make existing code compatible with WC 2.7 results in a lot of frustration: By replacing product method calls with global function calls in WC core, any existing overridden methods in product classes become useless, since WC now calls the global functions directly.

To make existing code compatible with WC 2.7, it will be necessary to rewrite all that functionality using always-on, type-specific filters provided in the (now global scope) functions. This is the definition of a breaking change and the required "fix" is a step back, since it introduces an overhead that I find unnecessary. Inheritance is a beautiful concept that works.

I'm open to discuss the implications of moving these methods by looking at each one individually. The only understandable reasons for making such a decision are:

  1. Move methods that contain templating code which is NOT (potentially or apparently) product-type-specific.
  2. Move methods that contain logic that depends on CRUD, which need to be abstracted.

Here's a short list of product methods that are potentially overridden by type-specific code and are eaten up by a black hole in master for no real reason:

  • get_availability -- A template function??? Doesn't get any more type-specific than this for complex product types. If availability strings are a templates-thing only, then why keep get_price_html in product scope? add_to_cart_text? get_image? If there is a need to introduce a stock.php template file, make it use the exiting product functions, which can be overridden by custom product types (re-write them if necessary). Currently, the logic associated with availability is scattered between product scope (stock functions), global scope (wc_format_stock_for_display) and templates! A big step back from 2.6.
  • get_availability_text -- Same as above.
  • get_availability_class -- Same.
  • get_price_suffix -- Very likely candidate for type-specific code. Commonly overridden by complex product types. Even WC core needs type-specific code in there: #12494

Some more "weird" decisions:

  • get_price_html_from_text -- Deprecated with no alternative (!). Why not just move it to global scope as a templating function (it can be considered as one). OK -- it's not used in core anymore, so deprecate it and move it to global scope to indicate there is an intention to keep it. It's a utility function used very commonly by extensions, so it makes sense to keep the associated strings in the core .pot file to encourage consistency. See get_price_html_from_to below, too (a different decision there!).
  • get_total_stock -- This one is interesting: "Use get_stock_quantity on each child. Beware of performance issues in doing so". Is the functionality useful or not? It's not used in core anymore, but is there any point in actually keeping it for any code that uses it? Deprecating it shows an intention to remove it in the future on the basis that it's not used in core, which is not always a convincing argument (this looks like a very useful utility method - see has_all_attributes_set below).

Some good decisions (assuming that changes are necessary):

  • get_price_html_from_to -- Offers template-related functionality so it can be moved to global scope and kept there.
  • All CRUD / strictly template-related methods (see edit below)...

And some decisions that are unnecessary but can be justified with a bit of head-scratching:

  • get_price_including_tax -- It's product related so I don't get the move to global scope 100%. Again, any custom product class that overrides it is doomed, but I haven't seen any custom types override this one.
  • get_price_excluding_tax -- Same.
  • get_display_price -- Same?
  • has_all_attributes_set -- Was accepted only a few months ago as a useful method and it's now deprecated with no alternatives. OK WC core doesn't use it -- but it was accepted on the basis of being useful to third parties. What changed? If WC had since tags in its methods this decision would really look funny (happy to see WC 2.7 finally introduces since tags a bit more consistently).

It seems to me that methods that don't directly use product (meta) data have simply been kicked out, without evaluating the potential usefulness/necessity of keeping some of them in product scope (those listed here). The ability to override a method is a central concept of inheritance -- dumping this wonderful "tool" in favor of using filters is... wrong (in the cases summarized above).

Please reconsider.

ref: #12065

@thenbrent @maxrice @helgatheviking @mikejolley @claudiosanches @justinshreve @mattyza

EDIT: Edited to make it clear I'm only referring to the listed methods (and not others). Other extension authors may have different feelings about specific methods being no longer override-able. I'm listing the ones I feel are ideally-suited for overriding in child classes.

@franticpsyx franticpsyx changed the title from [2.7] Product functions moved to global scope not backwards compatible to [2.7] Product functions moved to global scope not backwards compatible or meaningful Nov 30, 2016

@mikejolley

This comment has been minimized.

Member

mikejolley commented Nov 30, 2016

We're going to have to chat though each of these individually. I don't have time to go through all this second because I'm on support week, but get_availability and it's 2 supporting methods - lets do those.

get_availability You say it's type specific. Let's summarise what it actually does. In it's simplest form it converts a CORE stock status (in stock or out of stock) to a CSS classname (used only for template files and styling) and a 'human' version of the string, again only output in template files. The method itself is only called from template files.

The resulting CSS class and string is affected by:

  • stock status
  • backorder status (note this is going to be a stock status too in future versions #11259)

The actual displayed HTML for the availability should be consistent across product types. Any changes can be done via the provided filters which are in place still and can be used for any product type.

Unless your special product is introducing it's own stock status (something I don't think we support?), why does it need to be type specific @franticpsyx?

@mikejolley

This comment has been minimized.

Member

mikejolley commented Nov 30, 2016

get_price_html_from_text -- Deprecated with no alternative (!). Why not just move it to global scope as a templating function (it can be considered as one). OK -- it's not used in core anymore, so deprecate it and move it to global scope to indicate there is an intention to keep it. It's a utility function used very commonly by extensions, so it makes sense to keep the associated strings in the core .pot file to encourage consistency. See get_price_html_from_to below, too (a different decision there!).

Sorry but this one sounds pointless? If core has no use for a function, only a small subset of extensions require it, and those extensions need to update the deprecated call to a function based call anyway, why can those extensions not have their own function which handles that logic?

Having unused functions in core is the very definition of bloat and ultimately those functions are neglected. A function which outputs the word 'from' seems terribly wasteful. This is legacy from the old days of showing variation prices with that prefix.

@franticpsyx

This comment has been minimized.

Member

franticpsyx commented Nov 30, 2016

Just to get it out of the way, I am not necessarily looking at changes from an "own product" perspective. Some of the listed ones are not overridden in any class I've written.

You say it's type specific. Let's summarise what it actually does. In it's simplest form it converts a CORE stock status (in stock or out of stock) to a CSS classname (used only for template files and styling) and a 'human' version of the string, again only output in template files. The method itself is only called from template files.

This is a valid point - get_availability by itself does nothing (since 2.5 or was it 2.6), it calls get_availability_text and get_availability_class. It's used in templates only and doesn't directly rely on any product methods.

But: get_availability_text and get_availability_class call product methods. If you wanted to introduce a custom stock status for specific product types and currently override those, what's your option now? Use filters?

The actual displayed HTML for the availability should be consistent across product types. Any changes can be done via the provided filters which are in place still and can be used for any product type.

There is a conceptual difference between using inheritance (which was possible until now) and using filters. Inheritance allows you to do a number of things, such as use a different filter to modify the output of an overridden method in cases where this is desirable. Filters are better for general purpose tasks, such as templating.

Filters are more fragile than inheritance and can be overwritten at a later priority.

Unless your special product is introducing it's own stock status (something I don't think we support?)

What does "we support" mean in this context? We are talking about an existing API, not a feature that's no longer supported.

Introducing an extra availability "state" by changing the text - say, "Insufficient stock", is not a template thing. If a product type needs an extra stock status/state, it makes sense to keep at least get_availability_text and get_availability_text in product context to avoid breakage (at least until WC includes an API for introducing custom product stock statuses).

Suggesting the use of filters as an alternative is not helpful for the reasons outlined.

So why not rewrite the content of these methods and introduce a template, similar to what's done with get_price_html?

Or why not ALSO move get_price_html and a bunch of other methods to global scope? I think there's a fine line between using inheritance vs using filters when it comes to custom types, and for the reasons outlined, get_availability_text and get_availability_class are not too different from get_price_html in this sense. So, to me it makes more sense to keep the power of inheritance for them (and back-compat, as well).

@franticpsyx

This comment has been minimized.

Member

franticpsyx commented Nov 30, 2016

Sorry but this one sounds pointless? If core has no use for a function, only a small subset of extensions require it, and those extensions need to update the deprecated call to a function based call anyway, why can those extensions not have their own function which handles that logic?

See the entire text. Perhaps I'm wrong, but has_all_attributes_set was recently accepted on the basis of being useful as a utility method.

Having unused functions in core is the very definition of bloat and ultimately those functions are neglected. A function which outputs the word 'from' seems terribly wasteful. This is legacy from the old days of showing variation prices with that prefix.

I explained the reasons for keeping it as a global function here:

it makes sense to keep the associated strings in the core .pot file to encourage consistency. See get_price_html_from_to below, too (a different decision there!).

By (eventually) removing it, every plugin that used it will need to use its own localization context for a very widely used string. I can count at least 5 extensions that display "From:". There's no maintenance overhead with keeping this as a global method. Not keeping this means that every plugin/extension that uses it will then need to have its own style/translation context to display the same thing.

@mikejolley mikejolley modified the milestone: 2.7 Nov 30, 2016

@franticpsyx franticpsyx changed the title from [2.7] Product functions moved to global scope not backwards compatible or meaningful to [2.7] Some product functions moved to global scope not backwards compatible or meaningful Nov 30, 2016

@mikejolley mikejolley self-assigned this Dec 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment