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

Fix return type of DOMXPath::query #10443

Merged
merged 1 commit into from Dec 2, 2023
Merged

Fix return type of DOMXPath::query #10443

merged 1 commit into from Dec 2, 2023

Conversation

nielsdos
Copy link
Contributor

@nielsdos nielsdos commented Dec 2, 2023

This can also return namespace nodes, which are not a child class of DOMNode.

This can also return namespace nodes, which are not a child class of
DOMNode.
@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Dec 2, 2023
@orklah
Copy link
Collaborator

orklah commented Dec 2, 2023

Thanks!

@orklah orklah merged commit f7df43a into vimeo:5.x Dec 2, 2023
47 of 50 checks passed
@veewee
Copy link
Contributor

veewee commented Dec 4, 2023

@nielsdos Since you are doing a lot of work in DOM. Can you tell me if there is any specific reason why the namespace node is not a DOMNode?
I tried patching it a while back, but it seemed to scare people:

Reference : php/php-src#6939

@nielsdos
Copy link
Contributor Author

nielsdos commented Dec 4, 2023

@nielsdos Since you are doing a lot of work in DOM. Can you tell me if there is any specific reason why the namespace node is not a DOMNode? I tried patching it a while back, but it seemed to scare people:

Reference : php/php-src#6939

I'll reply here for now but let's not disturb this place here too much. You can contact me at my nielsdos at php dot net email address if you want to discuss more in detail.

I wasn't there when the decision was made 20 years ago. This is something that I would like to do though and I think it's possible to do. It's just not entirely clear what consequences this could have for the ecosystem (extensions+existing code).
But here's what I know:

  • The reason there is a separate class to begin with is because the underlying data structure in C for namespaces is completely different. This isn't much of a problem nowadays though as I rewrote a part of this code to fix crash bugs.
  • There are places where we should not allow DOMNameSpaceNodes (e.g. append etc). Now those functions take a DOMNode so we have to add restrictions at the right places.
  • Other extensions (both 1st and 3rd party) can rely on the fact that namespaces are not passed in.

tcarrio pushed a commit to open-feature/php-sdk that referenced this pull request Dec 4, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [vimeo/psalm](https://togithub.com/vimeo/psalm) | `~5.16.0` ->
`~5.17.0` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/vimeo%2fpsalm/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/vimeo%2fpsalm/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/vimeo%2fpsalm/5.16.0/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/vimeo%2fpsalm/5.16.0/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>vimeo/psalm (vimeo/psalm)</summary>

### [`v5.17.0`](https://togithub.com/vimeo/psalm/releases/tag/5.17.0)

[Compare
Source](https://togithub.com/vimeo/psalm/compare/5.16.0...5.17.0)

<!-- Release notes generated using configuration in .github/release.yml
at 5.x -->

#### What's Changed

##### Features

- Implement by-ref closure use analysis by
[@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10419

##### Fixes

- Update MongoDB stubs for 1.17 by
[@&#8203;alcaeus](https://togithub.com/alcaeus) in
[vimeo/psalm#10402
- Fix backtick analysis by [@&#8203;danog](https://togithub.com/danog)
in
[vimeo/psalm#10409
- Declare we do not support document highlighting by
[@&#8203;tm1000](https://togithub.com/tm1000) in
[vimeo/psalm#10408
- update define types to be correct by
[@&#8203;kkmuffme](https://togithub.com/kkmuffme) in
[vimeo/psalm#10423
- Fix return type of DOMXPath::query by
[@&#8203;nielsdos](https://togithub.com/nielsdos) in
[vimeo/psalm#10443
- Fix CLI -r error by [@&#8203;kkmuffme](https://togithub.com/kkmuffme)
in
[vimeo/psalm#10442
- Use correct file path while adding unused suppressions for virtual
\__… by [@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10434
- Restore support for null coalesce on match expressions by
[@&#8203;tscni](https://togithub.com/tscni) in
[vimeo/psalm#10422
- Fix static magic method pureness not being inherited from traits by
[@&#8203;tscni](https://togithub.com/tscni) in
[vimeo/psalm#10429
- Fix POSIX only detection of absolute paths by
[@&#8203;rarila](https://togithub.com/rarila) in
[vimeo/psalm#10441
- Emit UnusedPsalmSuppress issues for suppressed issues already removed
by [@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10431
- Use keyed arrays when assigning literal union keys & assertion fixes
by [@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10439

##### Internal changes

- use branch name to randomize tests to ensure test dependencies will
error by [@&#8203;kkmuffme](https://togithub.com/kkmuffme) in
[vimeo/psalm#10424

#### New Contributors

- [@&#8203;nielsdos](https://togithub.com/nielsdos) made their first
contribution in
[vimeo/psalm#10443

**Full Changelog**:
vimeo/psalm@5.16.0...5.17.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-feature/php-sdk).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44MS4zIiwidXBkYXRlZEluVmVyIjoiMzcuODEuMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
tcarrio pushed a commit to open-feature/php-sdk that referenced this pull request Dec 5, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [vimeo/psalm](https://togithub.com/vimeo/psalm) | `~5.16.0` ->
`~5.17.0` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/vimeo%2fpsalm/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/vimeo%2fpsalm/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/vimeo%2fpsalm/5.16.0/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/vimeo%2fpsalm/5.16.0/5.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>vimeo/psalm (vimeo/psalm)</summary>

### [`v5.17.0`](https://togithub.com/vimeo/psalm/releases/tag/5.17.0)

[Compare
Source](https://togithub.com/vimeo/psalm/compare/5.16.0...5.17.0)

<!-- Release notes generated using configuration in .github/release.yml
at 5.x -->

#### What's Changed

##### Features

- Implement by-ref closure use analysis by
[@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10419

##### Fixes

- Update MongoDB stubs for 1.17 by
[@&#8203;alcaeus](https://togithub.com/alcaeus) in
[vimeo/psalm#10402
- Fix backtick analysis by [@&#8203;danog](https://togithub.com/danog)
in
[vimeo/psalm#10409
- Declare we do not support document highlighting by
[@&#8203;tm1000](https://togithub.com/tm1000) in
[vimeo/psalm#10408
- update define types to be correct by
[@&#8203;kkmuffme](https://togithub.com/kkmuffme) in
[vimeo/psalm#10423
- Fix return type of DOMXPath::query by
[@&#8203;nielsdos](https://togithub.com/nielsdos) in
[vimeo/psalm#10443
- Fix CLI -r error by [@&#8203;kkmuffme](https://togithub.com/kkmuffme)
in
[vimeo/psalm#10442
- Use correct file path while adding unused suppressions for virtual
\__… by [@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10434
- Restore support for null coalesce on match expressions by
[@&#8203;tscni](https://togithub.com/tscni) in
[vimeo/psalm#10422
- Fix static magic method pureness not being inherited from traits by
[@&#8203;tscni](https://togithub.com/tscni) in
[vimeo/psalm#10429
- Fix POSIX only detection of absolute paths by
[@&#8203;rarila](https://togithub.com/rarila) in
[vimeo/psalm#10441
- Emit UnusedPsalmSuppress issues for suppressed issues already removed
by [@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10431
- Use keyed arrays when assigning literal union keys & assertion fixes
by [@&#8203;danog](https://togithub.com/danog) in
[vimeo/psalm#10439

##### Internal changes

- use branch name to randomize tests to ensure test dependencies will
error by [@&#8203;kkmuffme](https://togithub.com/kkmuffme) in
[vimeo/psalm#10424

#### New Contributors

- [@&#8203;nielsdos](https://togithub.com/nielsdos) made their first
contribution in
[vimeo/psalm#10443

**Full Changelog**:
vimeo/psalm@5.16.0...5.17.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-feature/php-sdk).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44MS4zIiwidXBkYXRlZEluVmVyIjoiMzcuODEuMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Tom Carrio <tom@carrio.dev>
@staabm
Copy link
Contributor

staabm commented Dec 30, 2023

I tried adding this change into phpstan, which sparked some interessing questions:

phpstan/phpstan-src#2842 (comment)

@nielsdos do you have concrete "real world" code examples for which this change is necessary/useful?

@nielsdos
Copy link
Contributor Author

I tried adding this change into phpstan, which sparked some interessing questions:

phpstan/phpstan-src#2842 (comment)

@nielsdos do you have concrete "real world" code examples for which this change is necessary/useful?

https://3v4l.org/GpOJA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants