Skip to content

Conversation

@Grotax
Copy link

@Grotax Grotax commented Jul 5, 2025

In addition to alexdebril#422
alexdebril#422 - partially corrupted the formation of Link for the Atom feed.
I learned about this only the day before yesterday and urgently made this PR.

@Grotax Grotax changed the title Import pr 431 [5.3] Fix: Analysis of relative links for the Atom feed Jul 5, 2025
@Grotax Grotax requested review from SMillerDev and Copilot July 5, 2025 09:02
Copilot

This comment was marked as outdated.

@Grotax Grotax force-pushed the import-pr-431 branch 3 times, most recently from 054d4f0 to f40ea68 Compare July 5, 2025 09:26
@Grotax Grotax requested a review from Copilot July 5, 2025 09:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes how relative links are resolved in Atom feeds by preserving the original item link for analysis and applying a base URL when needed.

  • Introduce linkForAnalysis in Node to keep the original link separate from the resolved link
  • Update Atom Link rule to prepend the host when resolving relative href values
  • Ensure the XML parser seeds new items with the original link for proper relative resolution
  • Add a comprehensive suite of tests covering various relative, absolute, protocol‐relative, fragment, query, and empty‐href scenarios

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/FeedIo/Rule/Atom/LinkTest.php New tests for resolving relative, absolute, protocol‐relative, fragment, query, and empty hrefs in Atom <link> elements
src/FeedIo/Rule/Atom/Link.php Updated logic to detect relative URLs and prepend the base host
src/FeedIo/Parser/XmlParser.php Changed parsing to use setLinkForAnalysis instead of setLink to seed original link
src/FeedIo/Feed/Node.php Added linkForAnalysis property and accessors, updated host extraction and content rewriting to use it
CHANGELOG.md Documented the fix under "Fixed"
Comments suppressed due to low confidence (3)

CHANGELOG.md:14

  • There are stray blank list markers above and an extra + prefix—clean up the bullet formatting to be a single - Fix: Analysis of relative links for the Atom feed (#10) entry.
- Fix: Analysis of relative links for the Atom feed (#10)

src/FeedIo/Feed/Node.php:145

  • The signature allows a default null but does not declare ?string $link, which can lead to type‐checking issues. Change to public function setLink(?string $link = null): NodeInterface for strict null safety.
    public function setLink(string $link = null): NodeInterface

src/FeedIo/Feed/Node.php:154

  • Similarly, this method should declare its parameter as nullable (?string $link) to match the default null and avoid runtime type warnings.
    public function setLinkForAnalysis(string $link = null): NodeInterface

return;
}

$itemFullLink = $this->getLinkForAnalysis();
Copy link

Copilot AI Jul 5, 2025

Choose a reason for hiding this comment

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

If linkForAnalysis is null, calling explode() on it below will raise a warning. Add an explicit null check before using it, or coerce to an empty string.

Suggested change
$itemFullLink = $this->getLinkForAnalysis();
$itemFullLink = $this->getLinkForAnalysis();
if (is_null($itemFullLink)) {
$itemFullLink = ''; // Coerce to an empty string to avoid runtime errors
}

Copilot uses AI. Check for mistakes.
Copy link

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Some small nits

IgorA100 and others added 3 commits July 12, 2025 18:19
Because the "Link" for the Atom feed will be corrupted.
Use the additionally created "LinkForAnalysis" object
Added analysis of relative links for the Atom feed
Use the new method "setLinkForAnalysis" instead of "setLink"
Co-authored-by: Sean Molenaar <[email protected]>
@Grotax Grotax merged commit a067950 into release/5.3.x Jul 12, 2025
4 checks passed
@Grotax Grotax deleted the import-pr-431 branch July 12, 2025 16:40
Grotax added a commit that referenced this pull request Aug 13, 2025
- Add logic to set the node's link from the <guid> element when isPermaLink="true" and no link is present. (#12)
- Do no longer default to 1800-01-01 as date for fetching feeds (#15)
- Don't set if-modified-since header when discover feeds (#19)

- Analysis of relative links for the Atom feed (#10)
- Implicit null deprecations fixed (#17)
Grotax added a commit that referenced this pull request Aug 13, 2025
Changed
- Add logic to set the node's link from the <guid> element when isPermaLink="true" and no link is present. (#12)
- Do no longer default to 1800-01-01 as date for fetching feeds (#15)
- Don't set if-modified-since header when discover feeds (#19)

Fixed
- Analysis of relative links for the Atom feed (#10)
- Implicit null deprecations fixed (#17)
@Grotax Grotax mentioned this pull request Aug 13, 2025
Grotax added a commit that referenced this pull request Aug 13, 2025
Changed
- Add logic to set the node's link from the <guid> element when isPermaLink="true" and no link is present. (#12)
- Do no longer default to 1800-01-01 as date for fetching feeds (#15)
- Don't set if-modified-since header when discover feeds (#19)

Fixed
- Analysis of relative links for the Atom feed (#10)
- Implicit null deprecations fixed (#17)
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.

3 participants