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

Fix news category breadcrumbs #4982

Merged
merged 1 commit into from Feb 16, 2023

Conversation

RichardBarrell
Copy link
Contributor

@RichardBarrell RichardBarrell commented Feb 14, 2023

The breadcrumbs for news categories were previously pointing to the category page but using the id of the news story instead of the id of the category.

Motivation and Context

The category breadcrumbs on /news.php?extend.${NEWS_ID} were pointing at /news.php?list.${NEWS_ID}.0 instead of /news.php?list.${NEWS_CATEGORY_ID}.0

Description

Pass an array to the e107::getUrl()->create invocation with the category's id instead of the story's id.

How Has This Been Tested?

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

Existing unit tests on master are already failing for me, so I'm not touching that.

Deltik helped me get the unit tests running, thank you!

The breadcrumbs for news categories were previously pointing to the
category page but using the id of the news story instead of the id of
the category.

Previously, if you had two news stories with ids 1 and 2, and one
category with id 1, then news story 1 would link to its category,
but news story 2 would link to a 404 page.
@Deltik Deltik merged commit 73405f5 into e107inc:master Feb 16, 2023
@CaMer0n
Copy link
Member

CaMer0n commented Feb 16, 2023

@RichardBarrell Thank you!

@Jimmi08
Copy link
Contributor

Jimmi08 commented Feb 19, 2023

@CaMer0n @Deltik

Category_sef value is not needed anymore? This PR limited data to ID field, name value is missing.

@Deltik
Copy link
Member

Deltik commented Feb 19, 2023

Mm, you're right to question this change, @Jimmi08. The core_news_sef_full_url eUrlConfig is broken now that news_sef is lost. We ought to restore the old behavior, write a regression test, then make the breadcrumbs work for all the supported SEF formats.

@RichardBarrell
Copy link
Contributor Author

@Jimmi08 oh nooo! Sorry about that.

@Jimmi08
Copy link
Contributor

Jimmi08 commented Feb 19, 2023

@Deltik I think that problem was in that array content, not in code for URL. After sync - when I noticed this on gitter - I had to run core update, then old code (before this change) worked. - I couldn't add new category in admin area, there was still SQL error. After rerunning core update, everything worked. I just hadn't time to write more at that time. And this is used on many places so it would need to be changed there too...

@RichardBarrell no problem. I use this version only for testing, not on live sites. Without you I wouldn't know that I can run those tests locally. Something new for me :) Just my English is limited.

@Deltik
Copy link
Member

Deltik commented Feb 19, 2023

I found ba18155 by @CaMer0n, which replaced category_id with id, but category_id was correct and id was not.

At this point:

url.php:138, core_news_url->create()
application.php:2097, eRouter->configCallback()
application.php:2250, eRouter->assemble()
e107Url.php:56, eUrl->create()
news.php:110, news_front->setBreadcrumb()
news.php:79, news_front->__construct()
news.php:2025, require_once()
news.php:23, {main}()

The value I have for $params is:

array (
  'news_id' => '5',
  'news_title' => 'Papilion Minter Savior',
  'news_sef' => 'papilion-minter-savior',
  'news_body' => '[html]<p>Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis. Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat.</p>[/html]',
  'news_extended' => '[html]<p>Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis. Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis</p>[/html]',
  'news_meta_title' => '',
  'news_meta_keywords' => 'fashion,food,slider',
  'news_meta_description' => 'Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis. Ut wisi enim ad minim...',
  'news_meta_robots' => '',
  'news_datestamp' => '1464637347',
  'news_modified' => '0',
  'news_author' => '1',
  'news_category' => '3',
  'news_allow_comments' => '0',
  'news_start' => '0',
  'news_end' => '0',
  'news_class' => '0',
  'news_render_type' => '0',
  'news_comment_total' => '0',
  'news_summary' => '',
  'news_thumbnail' => '{e_THEME}voux/install/2_.jpg,{e_THEME}voux/install/8.jpg',
  'news_sticky' => '0',
  'news_template' => NULL,
  'user_id' => '1',
  'user_name' => 'admin',
  'user_customtitle' => '',
  'user_image' => '',
  'user_login' => '',
  'category_id' => '3',
  'category_name' => 'Fashion',
  'category_sef' => 'Fashion',
  'category_icon' => '',
  'category_meta_keywords' => 'fashion',
  'category_meta_description' => '',
  'id' => '5',
  'page' => NULL,
)

id is 5 because that's the news_id, but it ought to be 3 for the category_id. Now we have to figure out what the intention was with e107_plugins/news/news_categories_menu.php as the comment in the commit mentioned.

Deltik added a commit to Deltik/e107 that referenced this pull request Feb 19, 2023
@Deltik
Copy link
Member

Deltik commented Feb 19, 2023

Got a fix ready for review: #4984

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.

None yet

4 participants