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

Add HttpInterface methods and add up-casts for type safety #3746

Merged
merged 4 commits into from
Aug 21, 2016
Merged

Add HttpInterface methods and add up-casts for type safety #3746

merged 4 commits into from
Aug 21, 2016

Conversation

Vinai
Copy link
Contributor

@Vinai Vinai commented Mar 12, 2016

This PR does not add any functionality. Its only purpose is to make the usage of the HttpInterface response more type safe.
As far as I can tell there should be no backward incompatible changes, except for the HTTP related methods from \Magento\Framework\Controller\AbstractResult being moved into a new \Magento\Framework\Controller\AbstractHttpResult child class.

All core classes extending AbstractResult but referencing HTTP methods now extend the new AbstractHttpResult. Any non-web result type can still extend AbstractResult without having applyHttpHeaders() caled during rendering.

Please reference issue #3737 for further information.

@adragus-inviqa
Copy link
Contributor

@Vinai - Are we sure we want a backwards incompatible change yet again? Why is this practice encouraged?

@Vinai
Copy link
Contributor Author

Vinai commented Mar 13, 2016

@adragus-inviqa I see your point. I for one still think the platform could use more cleanup. It may be a bit OCB, but every time I see classes depending on an interface, but calling methods on a concrete implementation that are not part of that interface, it just strokes me the wrong way.
So I would like to see more small steps in the direction of a "cleaner" code base. I mean, what is the alternative? Keep everything as it is? Better to make use of semver and keep things moving IMHO.

Question: Have you used custom result types that relied on AbstractResult setting HTTP specific properties? Otherwise there should be no BC break.

That said, the changes to the AbstractResult could be taken out of this PR again of course. It would be a pity, but better then nothing I guess.

@adragus-inviqa
Copy link
Contributor

I was half-venting, as the amount of BC-breaks in M2 is ridiculous. And BCs are not only about refactoring and clean code - it's about trust (actual, human trust), as I'm sure you know. And, in its current state, it's low. Sea-level-low.

Unfortunately, I see no way around some of the issues in here.

Maybe things are going to slow down in terms of BC in the future, but until then, I'm mentally working on Magento 13.46.

@Vinai
Copy link
Contributor Author

Vinai commented Mar 14, 2016

I do hope things will get (a lot) more stable in time.
I'm genuinely curious: do you think the current status quo should be already kept as it is with the goal of preserving BC? Is it "good enough" for that already, or does it need further work until it should be allowed to stabilize? I would really like to hear your opinion.

@adragus-inviqa
Copy link
Contributor

BiC = backwards-incompatible change

I hate BiCs during one major version, period.
"Good enough" was already decided for us, when Magento got out of alpha, right?
That's the thing the rubs me the wrong way: we shouldn't be talking about BiCs at this point at all.

But when M2 goes from app/code to Composer packages during a beta, people start thinking it's normal.

I do think the codebase needs quite a bit of refactoring, but I don't know how to do that without breaking my own rule (to not create any more BiCs). So I think it boils down to the amount of people impacted.

@Vinai
Copy link
Contributor Author

Vinai commented Mar 14, 2016

Thanks @adragus-inviqa. I also agree that Beta was just like Dev.

/**
* @var int
*/
protected $httpResponseCode;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking change

@Vinai
Copy link
Contributor Author

Vinai commented Mar 15, 2016

Removed the most BC breaking part of the PR, @antonkril

@paliarush paliarush self-assigned this May 5, 2016
@paliarush
Copy link
Contributor

Internal ticket created: MAGETWO-52571

/**
* Set a header
*
* If $replace is true, replaces any headers already defined with that
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Please add/fix description for all new methods in the interface. E.g. here it does not make sense to split lines 23 and 24
  2. Please change description of these methods in implementing classes to {@inheritdoc}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Vinai
Copy link
Contributor Author

Vinai commented May 8, 2016

Thank you for your review. I've applied the changes you suggested or replied to your comments in 2 cases.

@antonkril
Copy link
Contributor

antonkril commented Aug 9, 2016

While Result/Response API still requires bigger rethinking, this PR looks like the best we can do now from prospective of price (breaking changes)/value comparison.

@sshrewz sshrewz added the linked label Aug 9, 2016
@Vinai
Copy link
Contributor Author

Vinai commented Aug 11, 2016

Rebased onto develop HEAD and resolved merge conflicts.

@mmansoor-magento mmansoor-magento merged commit c930932 into magento:develop Aug 21, 2016
mmansoor-magento pushed a commit that referenced this pull request Aug 21, 2016
…s for type safety #3746

 - Merge branch 'httpinterface-methods' of https://github.com/Vinai/magento2 into Vinai-httpinterface-methods
mmansoor-magento pushed a commit that referenced this pull request Aug 21, 2016
…s for type safety #3746

 - Merge commit 'refs/pull/3746/head' of https://github.com/magento/magento2 into develop
mmansoor-magento pushed a commit that referenced this pull request Aug 21, 2016
…s for type safety #3746

 - suppress coupling warning until refactoring can be performed
mmansoor-magento pushed a commit that referenced this pull request Aug 21, 2016
…s for type safety #3746

 - Merge branch 'develop' into okapis-pr-branch
@vkorotun vkorotun added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development and removed linked labels Aug 22, 2016
magento-engcom-team pushed a commit that referenced this pull request Feb 13, 2019
[pangolin]Limit allowed bin/magento commands to specific list (MTF)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants