Fix issue 27162 sort order link incorrect compare

[mrtuvn]

Description (*)

Previously sortOrder number of links not working as aspect. Lower number mean start first. Higher number will stay at last. Seem issue caused by previous changes. Just correct it again

Fixed Issues (if relevant)

1. Fixes SortOrder is reversed in layout XML #27162: SortOrder is reversed in layout XML

Manual testing scenarios (*)

1. ...
2. ...

Questions or comments

After this changes can make order of some links in customer navigation account changed

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 are green)

[m2-assistant]

Hi @mrtuvn. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[lbajsarowicz]

Nice catch!
Could you provide very basic unit test for that?

[mrtuvn]

@lbajsarowicz ok i will check with fail tests

[mrtuvn]

@lbajsarowicz updated changes with test unit

[lbajsarowicz]

OK, so that means we have Unit Tests covering that feature :-)

✔️ Thank you for your contribution.

[magento-engcom-team]

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7147 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[engcom-Alfa]

Hi @mrtuvn.
Your changes work, but an order of all links in the navigation menu was changed

Actual Result:
❌

Expected Result:
✔️ order of links on mainline

Maybe you should sort the links in the right order?
@mrtuvn @lbajsarowicz Could you take a look?

Thanks!

Pull Request state was updated. Re-review required.

[mrtuvn]

@engcom-Alfa yes this because sort order defined in layout xml. I believe current the order of My account link is highest value
My Account
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Customer/view/frontend/layout/customer_account.xml#L27
My Order
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Sales/view/frontend/layout/customer_account.xml#L15
we need to re-change order value to make sure user have better experience

[mrtuvn]

hi guys any news on this

[lbajsarowicz]

Code looks fine. We need confirmation from QA team if no side effects were introduced.

[m2-assistant]

Hi @mrtuvn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[ihor-sviziev]

Looks like this PR was reverted in a360c06
@naydav could you explain why it was reverted?

[sdzhepa]

Hello @ihor-sviziev

This PR has been reverted due to a regression bug found during testing of 2.4.1 MC-36526

Short info from the report:

After merging of #27340 the customer links in customer account menu on storefront are messed up in EE and B2B editions. The reason is that pull request #27340 reversed the sort order comparison, but reversed the sort order numbers in layout only for CE.

Actual Result - link positions are mixed up in b2b

Expected result - link positions are proper in b2b and ee editions.

Aditional Comment

... please consider to revise issues #27162 it is personal expectations on links to be ordered in another direction, it is not a defect.

[lbajsarowicz]

@sdzhepa Please consider that it's not a personal preference, but consistency across the platform. Every single place should be consistent with each another and sortOrder is displayed in ascending order, not descending across the platform.

These changes were performed for consistency purposes and if Magento Commerce employees do not follow the changes to adjust the commercial platform, maybe it's time for them to review what is going on with Magento Core (Open Source)?

[mrtuvn]

why not update order in other b2b or ee modules. I don't think revert code for fix is the good way
invitations, gift registry, reward ..etc and others no doubt should update

[ericmorand]

@sdzhepa how can having a reversed sort order in customer navigation be not a defect? This looks like a lazy move to ensure backward compatibility with modules that accepted that situation instead of raising an issue when their maintainers faced it. And considering that the modules you are trying to preserve are modules maintained by the core contributor teams (EE and B2B editions), it looks like a big conflict of interest situation when EE and B2B developers are considered as first-class citizen and CE ones are considered secondary.

There is a bug here. It needs to be fixed. And EE and B2B cores need to be fixed accordingly.

[mrtuvn]

@ericmorand I'm afraid this can take time and much more effort. Since I think a lot a code depend on this. One you changed on this you must change other place. Core team has reverted my commit to previously

[ericmorand]

@mrtuvn , I know. This is easy to fix on our side, but the way this issue is handled doesn't cast a glorious light over Magento 2 core maintainers. There is a bug here, you fixed it, and they refused it because it created issues on their side because they used a buggy implementation in the past instead of fixing it as soon as they noticed it. In other words: they force everybody else in the world to fix that issue in their custom code so that they don't have the burden of fixing it on their side.

It's a lack of respect to their community.

[ihor-sviziev]

Hi @ericmorand,
I got your point, you're totally right in terms of ce vs ee modules. But let's try to see wider.

Unfortunately such change will break not the only b2b or ee extensions , but a lot of custom extensions developed by other people.

If this change will be accepted, even to 2.5 (as it breaks at least some extensions - it's backward incompatible change) - all extensions that will be affected this change will need to be fixed. There wouldn't be an easy way to fix it both for new version and not breaking compatibility for earlier versions, so the only way will be preparing separate version of extension specially for 2.5 and above. That's not the thing that everybody expects from Magento with their backward compatibility policy.

In case if you have any idea to make this change not so destructive - please let us know.

[ericmorand]

@ihor-sviziev , I get your point but why is including a breaking change a problem with this specific issue but not a problem for all the previous releases of Magento 2 that included breaking changes?

Don't look any further than 2.4.1. It comes with breaking changes that force everybody to test their code and potentially change them. I would totally agree with you if you were able to point me to the discussions that validated the breaking changes of 2.4.1. Or all other previous releases that included some breaking changes - and there are many of them.

I'm entitled to my opinion: when core maintainers think it's more comfortable for them to include some breaking changes, they have no remorse including them even in patch releases. When they think it's less comfortable for them - like our current case - they refuse perfectly valid PRs.

I may be wrong of course. But that's how it looks like from that side of the project.

So my proposal is : release 2.4.2 that fixes this issue and create a breaking change. Like you did with 2.4.1.

[ihor-sviziev]

@ericmorand
Unfortunately when you’re writing some code to magento core you can’t know for sure how exactly it will affect custom extensions.

For instance in case of this PR it looked to me ok to accept such change, but it broke some magento commerce extensions. That’s basically good that it was catched before release.

I don’t really know why exactly some backward incompatible changes are getting merged to core and includes into patch versions, maybe that was some security issue behind, maybe some other reason, maybe just that case wasn’t noticed.

Would be good if you could provide some examples, so we as a community maybe could analyze them and write some automated tests in order to prevent similar cases in future.

Again, if you know how we can make changes from this PR backward compatible - please let me know, we’ll just accept such a pull request as a replacement to this one

[ericmorand]

@ihor-sviziev Magento 2 maintainers create breaking change release deliberately:

https://devdocs.magento.com/guides/v2.4/release-notes/backward-incompatible-changes/index.html

https://devdocs.magento.com/guides/v2.4/release-notes/backward-incompatible-changes/reference.html

Just for the two latest releases. I let you check the previous ones.

So it is not unnoticed. Why is this PR which also create a breaking change not treated with as much indulgence as a core PR that creates a breaking change ?

[ihor-sviziev]

Hi @ericmorand,
From what I see - there only one thing that significantly affected upgrades to 2.4.0 - required ElasticSearch. This decision was done by Magento Arcitects, so that's the question to them why they decided.

Currently I have quite strong feeling that we should not accept such change due to inability to make these changes backward compatible.

Could you contact me in Slack and we'll discuss it more detailed?

[hostep]

I'm ok with approving this for 2.5.0 but not for 2.4.2.

The first one is a major release and it is expected things to break, the second one is a minor release and even though Magento always seem to ship with small backwards incompatible breaking things in minor releases (especially on the frontend), we should really try to prevent this as much as possible, I already have to spend way too much time on such minor/"easy" upgrades (which are never really easy).

[gabrieldagama]

@ihor-sviziev @hostep @ericmorand @mrtuvn

We've discussed this issue at our Public Triage Meeting (look at our community calendar for participate), as suggested by other contributors it makes sense to target such changes to the next minor release. We also need to provide an appropriate fix to EE and B2B repositories.

Thanks for highlight and discuss it.

[ihor-sviziev]

@mrtuvn could you recreate pr with the same changes?

[ericmorand]

@gabrieldagama thanks a lot, it's great news.

[hostep]

@gabrieldagama : just to clarify, by "minor" you mean 2.X.0 right?

In my opinion we should speak about major versions in this case, the 2 is just a marketing version without any meaning.

• 2.4.0 translates to 4.0.0 (major version)
• 2.4.0-p1 translates to 4.0.1 (patch version)
• 2.4.1 translates to 4.1.0 (minor version)
• ...

[ihor-sviziev]

@hostep yes, for sure, it should be included in 2.5.0