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

Rework the behavior of favicon search #5839

Merged
merged 5 commits into from Nov 10, 2023

Conversation

jaden
Copy link
Contributor

@jaden jaden commented Nov 9, 2023

Closes #5838

Changes proposed in this pull request:

  • Use xpath queries to reduce the filtering necessary.
  • Don't call checkUrl since the href value will not always be a full URL.
  • Check for a base path in the HTML to be used if the favicon href doesn't start with a leading slash.

How to test the feature manually:

  1. Subscribe to sites with favicon link hrefs that don't include the domain.
  2. Verify that the correct favicon is loaded.

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

Use xpath queries to reduce the filtering necessary.

Don't call checkUrl since the href value will not always be a full URL.

Check for a base path in the HTML to be used if the favicon href doesn't
start with a leading slash.
@Alkarex Alkarex added this to the 1.23.0 milestone Nov 9, 2023
@Alkarex
Copy link
Member

Alkarex commented Nov 9, 2023

Thanks! Looks good.
Try to run make test-all

lib/favicons.php Outdated Show resolved Hide resolved
lib/favicons.php Outdated Show resolved Hide resolved
@Alkarex Alkarex merged commit 57f4692 into FreshRSS:edge Nov 10, 2023
1 check passed
@Alkarex
Copy link
Member

Alkarex commented Nov 10, 2023

Good improvement 👍🏻
Please add a line for you https://github.com/FreshRSS/FreshRSS/blob/edge/CREDITS.md

jaden added a commit to jaden/FreshRSS that referenced this pull request Nov 10, 2023
@jaden jaden deleted the 5838-adjust-favicon-search branch November 10, 2023 22:07
Alkarex pushed a commit that referenced this pull request Nov 10, 2023
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.

[BUG] Sites with non-absolute paths to favicons don't load
2 participants