On second thought, maybe a factory is what is needed, to split the responsability between reading the source and executing a JSON path query.

I prefer JsonCrawlerInterface as it was, because it doesn't constrain the format of the JSON input. If you want to make an implementation that reads nested arrays or even objects (with PropertyAccessor?), it's possible. When the data is an argument to the find method, that restricts the way it can work.

So I suggest a JsonCrawlerFactory class used as a service, with a method (parse?) that takes the JSON and returns a JsonCrawler instance, injecting the functions.

class JsonCrawlerFactory implements JsonCrawlerFactoryInterface
{
    public function parse(string|resource $data): JsonCrawlerInterface
    {
        $crawler = new JsonCrawler($data, $this->functions);
    }
}

$jsonPathFactory->parse('{"foo":true}')->find('$.foo');

If we want to be consistent and imagine that the input format can be a nested array, maybe the name JsonCrawlerInterface isn't right (since we're no longer always crawling a JSON string).

I like the factory idea, however I'm a bit conflicted. If we just look at the current interface without the implementation, I don't find it particularly clear where the data comes from, and this necessarily implies a statefull implementation (with all the drawbacks that implies). That said, I really like how it looks in the code snippet you shared.

Maybe we shouldn't put the annotation for $data on the interface but only on our implementation? Implementations would be free to check whatever type they want in their logic then. As stated above, this could mean that we should rename the interface to CrawlerInterface, defining a class capable to use JsonPath syntax to query any type of data if someone wants to.

SQLite JSONB, PostgresSQL JSONB and MongoDB BSON are other JSON-like binary formats that could be retrieved (raw) from the a database and queried with JSONPath. The same goes for Yaml documents. So abstracting the format of the input document is nice to have.

If we just look at the current interface without the implementation, I don't find it particularly clear where the data comes from

That's exactly the purpose of this interface: show what you can do with an object, no matter the way it's made.

and this necessarily implies a statefull implementation (with all the drawbacks that implies

The factory is a stateless service; it has the Json Path custom functions that will be injected. The crawler is an object, not a service: it's legitimate for it to contain a state.

Maybe we shouldn't put the annotation for $data on the interface but only on our implementation? Implementations would be free to check whatever type they want in their logic then.

This would makes the interface less useful, since not all implementations of the interface accept the same arguments. You can't easily replace one implementation with another.

As stated above, this could mean that we should rename the interface to CrawlerInterface.

This new name you're proposing is perfect. And the JsonCrawler class keeps its name: it's the implementation of the CrawlerInterface for JSON. The interface is in the JsonPath namespace, so there is no doubt of its context without repeating "Json" in its name.

Thank you for this complete feedback! I just implemented what we discussed. The factory is a bit empty for now, but this will be our extension point as soon as I have some time to implement custom functions 🙂 Feedback welcome about the factory method's name, I'm open to better suggestions

I reworked the PR to remove the factory and use a prototype approach. I added withRawData(), so CrawlerInterface becomes injectable. Is it better to you? It solves the initial problem, and I'll be able to add a container call to a future withCustomFunctions() method with a tagged iterator in a following PR.

I'm open for suggestions about the naming. I noted the suggestion here, but nor create or crawl seems right here: we don't create the crawler, and we don't crawl (yet) the JSON string/resource at this point.

Also, FWB integration added 👍

Initialize?

@OskarStark why not, but I'm not sure it's a perfect match: wither methods can be "semantically" called multiple times, where calling "initialize" multiple times could feel odd. Maybe initializeWith() is a compromise?

I think we should have both interface, that's a nice idea. This would solve the point you raise and still allow Jérôme's idea.

The suggested JsonCrawlerInterface looks weird to me.

Using it means you will have a property in your class holding a JsonCrawlerInterface on which you should never call the find() method (as it would not have any data to actually find something). And you would call $crawler = $this->crawler->fromJson($data) to get a working crawler.
To me, the factory being a separate class was making the code a lot more understandable.

Then maybe the too interfaces should be split:

• JsonCrawlerInterface with only a fromJson(): CrawlerInterface - that's the factory, but named in a more DX-friendly way
• CrawlerInterface, with the find method (and NOT autowirable)

Thanks, I updated the PR to have two interfaces. JsonCrawlerInterface is autowirable in FWB.

Fabbot complains about the TypeError message format. I'd say it is not valid, PHP internal type errors doesn't include type quoting.

Discussions makes me wonder: is there a design flaw maybe in the current state of the crawler? The number of comments on the PR makes me wonder so.
I took a look at ExpressionLanguage. We could take inspiration from it maybe. ExpressionLanguage takes providers (ExpressionFunctionProviderInterface[]) through the constructor.

This could be the same for JsonCrawler, with an array $functions or something similar provided through the constructor. Then we could also change the current JsonCrawler::find($path) so it accepts the data directly, changing the signature to JsonPath::find($path, mixed $data) (args order can be discussed 🙂). We can keep an interface describing the find() method, this is a nice extension point as pointed by Jérôme.

Status: Needs work