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

Allow access to current order #1032

Merged
merged 2 commits into from
Jun 20, 2020
Merged

Allow access to current order #1032

merged 2 commits into from
Jun 20, 2020

Conversation

woutersamaey
Copy link
Contributor

It's a pain in Magento to get the order currently being created. This would be the cleanest solution, even though you could build on the checkout_type_onepage_save_order event, but I don't like that. You'd also need to make sure the event is always first and it created clutter.

Magento uses the registry and current_ prefix for everything, so why not here?

I understand that for Magento 2 this would be a big no-no, but for Magento 1 I think this is perfectly acceptable and would make a lot of people happy.

colinmollenhour
colinmollenhour previously approved these changes Jun 9, 2020
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

I like it. Have seen plenty of cases where each extension loads the order by ID just to check one attribute because there is no reference to the original object.

Thanks for the PR!

@sreichel sreichel added Component: Sales Relates to Mage_Sales new feature labels Jun 9, 2020
@tmotyl
Copy link
Contributor

tmotyl commented Jun 9, 2020

Will it work if there are few orders created in a batch?

@colinmollenhour
Copy link
Member

Will it work if there are few orders created in a batch?

How is that done in core currently?

I suppose we could add an unregister() before the register() to avoid any errors being thrown in case some extension uses quote service to create multiple orders.

@woutersamaey
Copy link
Contributor Author

woutersamaey commented Jun 10, 2020

An unregister would be needed, indeed. I'll make the change.

Added an "unregister" in case you're creating multiple orders per request
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

Nice improvement!

@woutersamaey
Copy link
Contributor Author

woutersamaey commented Jun 19, 2020

@sreichel Can we get this approved?

Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

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

LGTM

@tmotyl
Copy link
Contributor

tmotyl commented Jun 19, 2020

Funny, today I have a case In a project where this change ould be helpful - to avoid having to fetch order again from db.

@sreichel sreichel added performance Performance related and removed performance Performance related labels Jun 19, 2020
@sreichel sreichel merged commit 5622585 into OpenMage:1.9.4.x Jun 20, 2020
@sreichel sreichel added this to the Release 19.4.4 milestone Jun 26, 2020
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 20, 2020
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Sales Relates to Mage_Sales new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants