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 #606, correct URL for categories if Blog is on /home #804

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

lerni
Copy link
Contributor

@lerni lerni commented Jan 22, 2025

Description

Get correct URL for categories if Blog is on /home. For categories $this->Blog()->Link() returns just / but should return /home.

Manual testing steps

Create a Blog on /home and see categories fail

[User Warning] Request handler SilverStripe\CMS\Controllers\ModelAsController does not have a url_segment defined. Relying on this link may be an application error
GET /category/...

Line 573 in /var/www/html/vendor/silverstripe/framework/src/Control/RequestHandler.php

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

src/Model/BlogObject.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

Please also target this PR to the 4.3 branch so it can be released as a patch instead of waiting for the next minor release

@GuySartorelli
Copy link
Member

Please also update the commit message both to reflect the bug that is being fixed and to include the correct commit prefix (there's a link in the PR checklist for commit messages) - and check the checklist while you're at it. That checklist is there to help you as the contributor make sure you've got all these sorts of things correct and it's ready for review.

@GuySartorelli
Copy link
Member

Finally, if you could add a unit test for this that would be great!

@GuySartorelli GuySartorelli self-assigned this Jan 23, 2025
@lerni lerni changed the base branch from 4 to 4.3 January 24, 2025 06:54
@lerni lerni changed the title Correct URL for categories if Blog is on /home FIX #606, correct URL for categories if Blog is on /home Jan 24, 2025
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, works well locally and good unit test.
We don't usually include the issue URL in a PHPDoc but it's not worth delaying merging for that.
Thanks for submitting this!

@GuySartorelli GuySartorelli merged commit f9a33f0 into silverstripe:4.3 Jan 27, 2025
12 checks passed
@lerni lerni deleted the actionparm branch January 27, 2025 18:57
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