Skip to content

Replace Locator and Content\Type\Sniffer with Content\Detector#849

Draft
Art4 wants to merge 19 commits intosimplepie:masterfrom
Art4:introduce-content-detector
Draft

Replace Locator and Content\Type\Sniffer with Content\Detector#849
Art4 wants to merge 19 commits intosimplepie:masterfrom
Art4:introduce-content-detector

Conversation

@Art4
Copy link
Contributor

@Art4 Art4 commented Sep 13, 2023

In #731 we plan to deprecate the SimplePie\File class. The file class is required for SimplePie\Locator and SimplePie\Content\Type\Sniffer, so the plan is to deprecate this classes too. See also the discussion in #826 (comment)

This PR introduces a new interface SimplePie\Content\Detector that defines the most features of the Locator and Sniffer in 3 methods.

The SimplePie\Content\VerfiedFeedsDetector class was created for BC. It internally uses the old Locator and Sniffer classes. This is needed because Locator and Sniffer classes could be replaced by the user using the Registry.

A new SimplePie\Content\UnverfiedFeedsDetector has to be created to replace the function of Locator and Sniffer. The idea is that this Detector should not have a dependency to a HTTP client but should only work with Response classes.

This PR is under construction and requires some other PRs like #838 and a refactoring of the microformats handling in SimplePie\SimplePie::fetch_data()

This was referenced Sep 13, 2023
@Art4 Art4 mentioned this pull request Sep 21, 2023
48 tasks
@Art4 Art4 modified the milestones: 1.9.0, 1.10.0 Sep 30, 2024
use SimplePie\SimplePie;

/**
* Helper for feed auto-discovery and type sniffing
Copy link
Member

Choose a reason for hiding this comment

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

Why combine the two classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SimplePie\Locator internally depends on SimplePie\Content\Type\Sniffer, so by replacing the Locator class we also replace the Sniffer class.

}

/**
* Check if the response contains a feed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Check if the response contains a feed
* Check if the response body contains a feed

Copy link
Contributor Author

@Art4 Art4 Jun 27, 2025

Choose a reason for hiding this comment

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

The Detector looks also into the headers and the requested uri to check if the Response contains a feed.

public function discover_possible_feed_urls(Response $response, int $discovery_level = SimplePie::LOCATOR_ALL): array
{
/** @var Locator */
$locator = $this->registry->create(
Copy link
Member

Choose a reason for hiding this comment

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

Unlike before, this creates a new locator entry for each method call. But it probably is not very costly and we only call the methods few times anyway.

$discovered = null;
$checked_feeds = 0;

foreach ($possible_feed_urls as $href) {
Copy link
Member

Choose a reason for hiding this comment

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

I like having a pure (side-effect-free detector) but moving the effects to SimplePie class is not nice when it is already overly bloated. Could we have a separate helper class that would encapsulate this code?

@Art4 Art4 force-pushed the introduce-content-detector branch from 90564b9 to 0836f14 Compare June 27, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants