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.

Sign up for GitHub

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

Jump to bottom

Sort categories returned by "getChildren" by position #351

Merged
merged 3 commits into from
Oct 4, 2018
Merged

Sort categories returned by "getChildren" by position #351

merged 3 commits into from
Oct 4, 2018

Conversation

EliasKotlyar
Copy link
Contributor

@EliasKotlyar EliasKotlyar commented Oct 9, 2017 •
edited by tmotyl
Loading

Change "getChildren" category method to return categories sorted by position.
Fixes issue #350

LeeSaferite
LeeSaferite suggested changes Oct 9, 2017
View reviewed changes
Copy link
Contributor

@LeeSaferite LeeSaferite left a comment

Choose a reason for hiding this comment

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

Sorry I didn't include this in the first comment. From a code change POV I think the PR is fine with these adjustments to make it blend with the existing code better. It may introduce a merge issue further down the line due to changing the location of the semicolon on the where statements, but I think it's much cleaner overall.

My review is strictly code related and I have not personally tested the functionality. @drobinson, do you have time to test the PR functionality?

app/code/core/Mage/Catalog/Model/Resource/Category.php Outdated Show resolved Hide resolved
app/code/core/Mage/Catalog/Model/Resource/Category/Flat.php Outdated Show resolved Hide resolved
@LeeSaferite
Copy link
Contributor

This still needs a functional review from one of the other members as I do not currently have a good setup to test things (in the middle of moving).

@sreichel
Copy link
Contributor

sreichel commented Oct 11, 2017 •
edited
Loading

LGTM.

Question. How about adding a $sortByPosition flag like in M2s PR? (magento/magento2#11342)

@tmotyl tmotyl changed the title Fixxes #350 Change "getChildren" sort ordering - Fixes #350 Dec 13, 2017
@tmotyl tmotyl changed the title Change "getChildren" sort ordering - Fixes #350 Sort categories returned by "getChildren" by position Dec 13, 2017
@sreichel sreichel mentioned this pull request Jan 3, 2018
Closed
@sreichel
Copy link
Contributor

This still needs a functional review from one of the other members ...

  • it's not used in core
  • the change works as intended

I agree with this PR with still would suggest to use an optional flag (#429) so we can enure not breaking anything,

@sreichel sreichel added review needed Problem should be verified hold labels Jan 25, 2018
@colinmollenhour colinmollenhour merged commit 1f14c9a into OpenMage:1.9.3.x Oct 4, 2018
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Dec 6, 2018
@EliasKotlyar @edannenberg
[bug-OpenMage#351] Position attribute of child categories is ignored
9d5da18 
Categories returned by getChildren() should be sorted by position.

refs: OpenMage#350
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Feb 14, 2019
@EliasKotlyar @edannenberg
[bug-OpenMage#351] Position attribute of child categories is ignored
3206412 
Categories returned by getChildren() should be sorted by position.

refs: OpenMage#350
fplantinet pushed a commit to fplantinet/magento-lts that referenced this pull request Mar 27, 2019
@EliasKotlyar @fplantinet
Sort categories returned by "getChildren" by position (OpenMage#351)
0525ea7 
* Fixes OpenMage#350
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Apr 1, 2019
@EliasKotlyar @edannenberg
[bug-OpenMage#351] Position attribute of child categories is ignored
c68378d 
Categories returned by getChildren() should be sorted by position.

refs: OpenMage#350
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 22, 2019
@EliasKotlyar @edannenberg
[bug-OpenMage#351] Position attribute of child categories is ignored
4aacc62 
Categories returned by getChildren() should be sorted by position.

refs: OpenMage#350
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Oct 25, 2019
@EliasKotlyar @edannenberg
[bug-OpenMage#351] Position attribute of child categories is ignored
aeaf227 
Categories returned by getChildren() should be sorted by position.

refs: OpenMage#350
@colinmollenhour colinmollenhour mentioned this pull request May 21, 2020
Closed
@sreichel sreichel added the Component: Catalog Relates to Mage_Catalog label Jun 9, 2020
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 20, 2020
@EliasKotlyar @edannenberg
[bug-OpenMage#351] Position attribute of child categories is ignored
c807242 
Categories returned by getChildren() should be sorted by position.

refs: OpenMage#350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colinmollenhour colinmollenhour colinmollenhour approved these changes

@LeeSaferite LeeSaferite LeeSaferite approved these changes

@drobinson drobinson Awaiting requested review from drobinson

Assignees

@drobinson drobinson

Labels
Component: Catalog Relates to Mage_Catalog enhancement hold review needed Problem should be verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@EliasKotlyar @LeeSaferite @sreichel @colinmollenhour @drobinson @mw-ekotlyar