Skip to content

Fixed typo in method declaration #9154

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

Closed
wants to merge 4 commits into from

Conversation

ccasciotti
Copy link
Contributor

Fixed a typo in method declaration, changed proccessAdditionalValidation in processAdditionalValidation

@adragus-inviqa
Copy link
Contributor

Please don't change the API of AbstractCarrierInterface, as much as I want it, too.

Add a new method, deprecate the old one, and delegate old to new.

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov left a comment

Choose a reason for hiding this comment

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

Hi,
Unfortunately we can not accept this PR in it's current state due to the fact, that backwards compatibility policy prohibits renaming of methods.
Please refer to the comment #9154 (comment) if you really wish to implement the fix.

@ccasciotti
Copy link
Contributor Author

I've modified PR, please let me know if now it's ok. I have a question, in a future minor version (ex. 2.2) the API will be editable or it has to remain unchangeable?

@adragus-inviqa
Copy link
Contributor

@ccasciotti - thanks for the change. One more nit, if I may: can you tell others what they should be using instead? Something like @deprecated Use processAdditionalValidation() instead. It really helps.

Also, removing an interface method is a BC break, as other potential 3rd-party implementations will fail. And BC-breaks should be only made in major versions, not minor. Speaking of which, I don't think M2 considers the x in 2.x as the minor version. They don't do semver (otherwise, the next major version would be M3), and they don't have to. I'm not sure, but x in this case is considered the major version by M2.

I still don't know when they'll clean up all the deprecated code, though.

@@ -325,6 +325,7 @@ public function checkAvailableShipCountries(\Magento\Framework\DataObject $reque
*
* @param \Magento\Framework\DataObject $request
* @return $this|bool|\Magento\Framework\DataObject
* @deprecated
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
*/
public function proccessAdditionalValidation(\Magento\Framework\DataObject $request)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method's phpdoc should just be {@inheritdoc}, precisely to avoid duplication.

Copy link
Contributor Author

@ccasciotti ccasciotti Apr 11, 2017

Choose a reason for hiding this comment

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

I agree with you, change done.

@ccasciotti
Copy link
Contributor Author

ccasciotti commented Apr 11, 2017

@adragus-inviqa i've added the explanation on deprecated method.
Thanks for explanation of magento's major/minor version

@ishakhsuvarov i've implemented requested changes

@okorshenko okorshenko modified the milestones: April 2017, May 2017 May 9, 2017
@okorshenko okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017
@okorshenko
Copy link
Contributor

Hi @ccasciotti
Thank you for your contribution. Please, take a look at http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/
According to our policy, Introduction of a method to a class or interface is a prohibited code change.

Unfortunately, we can not modify existing @api interfaces.

@okorshenko okorshenko self-assigned this Jun 21, 2017
@okorshenko
Copy link
Contributor

Hi @ccasciotti
\Magento\Shipping\Model\Carrier\AbstractCarrierInterface and \Magento\Shipping\Model\Carrier\AbstractCarrier is a legacy implementation and should be deprecated in favor of \Magento\Shipping\Model\Carrier\CarrierInterface.
Unfortunatelly, we can not deprecate these interfaces right now because in order to implement shipping integration requires extending from AbstractCarrier.

So we may accept fix in abstract AbstractCarrier class but not in AbstractCarrierInterface and this won't be BiC. But there are no lots of benefits from such code change.

Thank you for your contribution. But we need to reject this PR.

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.

4 participants