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

Match attribute names in a case-insensitive manner. #219

Merged
merged 14 commits into from
Jan 3, 2024
Merged

Match attribute names in a case-insensitive manner. #219

merged 14 commits into from
Jan 3, 2024

Conversation

chrishow
Copy link
Contributor

Modifies Translator to perform case-insensitive matches on attribute names.

eg:

Given <div data-FOO='bar'>baz</div>, both these selectors match:
"[data-FOO='bar']"
"[data-foo='bar']"

Case-sensitivity is maintained for attribute values, such that these selectors do not match:
"[data-FOO='BAR']"
"[data-foo='BAR']"

@g105b
Copy link
Member

g105b commented Dec 30, 2023

Hi @chrishow thank you for your first contribution to the repository. Nice to meet you!

I've just reviewed this PR and I'm happy with it. It all looks good to me. Have you intentionally marked is as WIP? Do you intend to add anything to this, or are you happy for it to be merged?

I see that the current version of PHPUnit only runs with PHP 8.1 or higher, so I suggest we update the Github Action runner to use the same PHPUnit as composer.json. I will do this, and while I'm at it I will implement matrix builds to test on all available PHP versions (this involves upgrading the Action runner to a new version of PHP Unit that I also maintain).

@g105b
Copy link
Member

g105b commented Dec 30, 2023

I've just updated the CI workflow to run all versions of PHP. I'm happy to merge if you are - just confirming because of the draft status you added to the PR.

@g105b g105b added this to WIP in Overview via automation Dec 30, 2023
@chrishow
Copy link
Contributor Author

Hi!

I marked it as draft because this would break case-sensitive matching in XML.

Have a look at issue #220 and let me know what you think over there...

@g105b
Copy link
Member

g105b commented Jan 2, 2024

I actually think it would be worth merging this PR first, and then implementing the distinction of XML vs HTML as a separate PR afterwards, because the use case for HTMLDocuments is so much more common than working with XML (at least for me). I'd like to hear your thoughts on this @chrishow .

Also add tests for XML-mode case-sensitivity.
@chrishow
Copy link
Contributor Author

chrishow commented Jan 3, 2024

I agree that HTML-style matching is likely to be a much more common use case for this library. That's certainly the only way I'm using it...

So I have added an option to the constructor (bool $htmlMode) which defaults to true. If this is true, attribute names are matched in a case-insensitive manner.

I've also added tests for $htmlMode = false (ie, 'XML mode').

So to continue using Translator for HTML-style matching, no changes to existing code are required, but to match XML-style, an extra argument is needed, eg:

Ordered argument style:
new Translator("[data-FOO='bar']", ".//", false);

Named argument style:
new Translator("[data-FOO='bar']", htmlMode: false);

It's perhaps worth noting that for XML-style matching to work, you must load the document content with DOMDocument->load/DOMDocument->loadXML instead of DOMDocument->loadHTMLFile/DOMDocument->loadHTML, as the HTML loading methods automatically convert the tags and attribute names to lowercase.

@chrishow chrishow marked this pull request as ready for review January 3, 2024 11:46
@g105b
Copy link
Member

g105b commented Jan 3, 2024

I've just upgraded the minimum PHP version from 7.3 to 8.0 - even though 8.0 is out of date now, it's always a good idea to support as far back as possible. This change was necessary to utilise promoted constructor properties.

I will now run the unit tests on the PHP.Gt/Dom project, which heavily utilises this CssXPath library, to ensure no functionality has regressed.

@g105b
Copy link
Member

g105b commented Jan 3, 2024

Looks great:

https://github.com/PhpGt/Dom/actions/runs/7400933221

No issues with running your changes in PHP.Gt/Dom

I'll go ahead and merge.

@g105b g105b merged commit aa941bb into PhpGt:master Jan 3, 2024
13 checks passed
Overview automation moved this from WIP to Done January 2024 Jan 3, 2024
@g105b
Copy link
Member

g105b commented Jan 3, 2024

Thanks so much for your first contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Overview
Done January 2024
Development

Successfully merging this pull request may close these issues.

None yet

2 participants