Skip to content
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

Remove deprecation without alternative #11070

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Remove deprecation without alternative #11070

merged 1 commit into from
Dec 18, 2017

Conversation

schmengler
Copy link
Contributor

Description

The Cart and CartInterface have been deprecated a while ago without ever providing useful alternatives, causing much confusion amongst developers.

Later, deprecation guidelines were introduced:

Use the @deprecated tag to mark methods as deprecated and follow it up with an explanation.

Use the @see tag to recommend the new API to use instead of the old one.

We should update deprecation annotations accordingly and "un-deprecate" classes and methods that do not have any alternatives yet.

Fixed Issues (if relevant)

  1. Please add your expectations for @deprecated annotations #10133: Please add your expectations for @deprecated annotations

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@orlangur
Copy link
Contributor

I believe they were deprecated with some existing implementation in mind. Isn't \Magento\Quote\Api\CartItemRepositoryInterface a better alternative? (not saying Quote interfaces can do everything Cart did before at the moment).

@vrann
Copy link
Contributor

vrann commented Sep 26, 2017

@schmengler you are right that @deprected tag should always follow with the corresponding @see statement, but unfortunately, it is not an option to remove the tag. If the code marked as a deprecated, it means that some work was done already to introduce new mechanism and keeping current interface public and available will cause more broken clients when the code is removed.

In this particular case, CartInterface was replaced with the \Magento\Quote\Api\Data\CartInterface and corresponding implementation. Would you be willing to add @see tag instead?

@schmengler
Copy link
Contributor Author

@vrann that would be better than the status quo but after I talked to @antonkril yesterday, I'd like to suggest a third way:

Add a comment "Will be deprecated", as in AbstractBlock (but without adding @api)

The reason is, that the CartInterface and CartManagementInterface API lacks important functionality. For example, it is possible to create new cart items but you need to craft them manually. There is no sensible way to add complex product types to the cart.

That leaves us with only one reasonable way to add products to the cart, namely Cart::addProduct(). It should not be deprecated before the planned CartItemBuilder is implemented. Maybe some of the other methods in the cart model can be deprecated already, but not the whole class.

@vrann
Copy link
Contributor

vrann commented Sep 28, 2017

@schmengler that sounds like a reasonable compromise.
Would there be any issues with the fact that it is deprecated in 2.2 and un-deprecated in 2.3?

Please re-create your PR with the new changes.

@orlangur
Copy link
Contributor

Do we really need to un-deprecate it literally instead of adding comment regarding partially suiable interfaces + the note in some scenarios you still have to use this deprecated class?

Like \Magento\Payment\Model\Method\AbstractMethod is deprecated but still widely used in core.

@vrann vrann reopened this Sep 29, 2017
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 29, 2017

CLA assistant check
All committers have signed the CLA.

@schmengler
Copy link
Contributor Author

@vrann

Would there be any issues with the fact that it is deprecated in 2.2 and un-deprecated in 2.3?

Was the question for me? I don't see any issues with it :)

@orlangur

Do we really need to un-deprecate it literally instead of adding comment regarding partially suiable interfaces + the note in some scenarios you still have to use this deprecated class?

IMHO, yes. If you find methods that have a feasible alternative, you could mark them as deprecated individually. But the whole class, no. Meanwhile it's even covered with @api, which sort of contradicts the previous deprecation.

@orlangur
Copy link
Contributor

In this particular case, CartInterface was replaced with the \Magento\Quote\Api\Data\CartInterface and corresponding implementation. Would you be willing to add @see tag instead?

Currently PR lacks this information, maybe some other interfaces are also suitable. As to me class and interface must remain deprecated but contain maximum information regarding alternatives.

Let's wait for feedback from @vrann.

@vkublytskyi
Copy link

@schmengler Seems \Magento\Quote\Model\Quote::addProduct is an alternative to \Magento\Checkout\Model\Cart\CartInterface::addProduct. For me, this is more correct as checkout should be responsible for transforming quote/cart into an order and cart filling should be inside quote module.

@schmengler
Copy link
Contributor Author

@vkublytskyi you're right, that's a good alternative. It's slightly less convenient because you cannot just pass product id but that's okay, this makes it a more type safe interface.

Unfortunately it is not part of \Magento\Quote\Api\Data\CartInterface, the interface that \Magento\Quote\Model\Quote implements. But at least it is not deprecated.

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Quote/Api/Data/CartInterface.php is the only interface implemented by quote model and it is pretty useless here.

@orlangur orlangur self-assigned this Dec 15, 2017
@orlangur orlangur added this to the December 2017 milestone Dec 15, 2017
@magento-team magento-team merged commit e5479b4 into magento:2.2-develop Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants