Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Jul 16, 2025

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

This PR makes the JsonPath component RFC compliant by removing all skipped tests of the compliance test suite.

src/Symfony/Component/JsonPath/Nothing.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
@alexandre-daubois alexandre-daubois force-pushed the jp-compliance branch 2 times, most recently from d1364da to 99ff096 Compare July 17, 2025 10:00
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/Nothing.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/Nothing.php Outdated Show resolved Hide resolved
@alexandre-daubois
Copy link
Member Author

Both comments addressed @stof. Thanks!

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Quick review, I didn't comment on every line where a comment could apply, please take them as hints that may apply elsewhere

src/Symfony/Component/JsonPath/Nothing.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
@alexandre-daubois
Copy link
Member Author

I address CS in 3fad1f3 and performed many changes in comparison/ordering in 74bb566, thanks for the hints for the comparison

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM after these changes

src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
}
}
} else {
foreach ($left as $key => $value) {
Copy link
Member

Choose a reason for hiding this comment

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

this foreach means the order of the keys don't matter - is this covered by the test suite?

Copy link
Member Author

@alexandre-daubois alexandre-daubois Sep 9, 2025

Choose a reason for hiding this comment

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

Actually we can keep only this loop, the comparison is order independent (see https://datatracker.ietf.org/doc/rfc9535/ section 2.5.2.2)

src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
@alexandre-daubois
Copy link
Member Author

Comments addressed in b2e7b34, thanks for the extensive review

src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
src/Symfony/Component/JsonPath/JsonCrawler.php Outdated Show resolved Hide resolved
@alexandre-daubois alexandre-daubois force-pushed the jp-compliance branch 2 times, most recently from 21352ba to 0b5baee Compare September 9, 2025 08:27
@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit ce92504 into symfony:7.3 Sep 10, 2025
8 of 11 checks passed
@fabpot fabpot mentioned this pull request Sep 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.