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

news: Fix category link in both breadcrumb and menu #4984

Merged
merged 4 commits into from
Apr 4, 2023

Conversation

Deltik
Copy link
Member

@Deltik Deltik commented Feb 19, 2023

Motivation and Context

These changes fixed individual symptoms, but not the root cause:

  • ba18155 broke the news plugin category breadcrumb in an effort to fix the category list in the news_categories menu.
  • Fix news category breadcrumbs #4982 fixed that category breadcrumb but broke the friendly name for search engine-friendly URLs.

Description

This change reverts both failed fixes and actually addresses the root cause: When rendering the category list, read out the category_id, and e_news_category_item::sc_news_category_url() should be passing in category_id, not id. id cannot mean both the category ID and the news item ID.

How Has This Been Tested?

Manually, I checked both the news_categories menu behavior and the breadcrumb behavior on all news SEF variants at /e107_admin/eurl.php?mode=main&action=config.

Automated tests for the news plugin category SEF URLs are in the new file e107_tests/tests/unit/plugins/news/PluginNewsTest.php.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

@Deltik Deltik requested a review from CaMer0n February 19, 2023 22:55
@Deltik Deltik mentioned this pull request Feb 19, 2023
10 tasks
@codeclimate
Copy link

codeclimate bot commented Feb 20, 2023

Code Climate has analyzed commit 1d1f4d0 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 75.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 34.7% (0.0% change).

View more on Code Climate.

@CaMer0n
Copy link
Member

CaMer0n commented Apr 4, 2023

Excellent! Thanks @Deltik

@CaMer0n CaMer0n merged commit 5f492a8 into e107inc:master Apr 4, 2023
@Deltik Deltik deleted the hotfix/4982 branch April 4, 2023 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants