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

SimpleXMLElement regressions on Psalm 5.15 #10204

Closed
edsrzf opened this issue Sep 14, 2023 · 3 comments
Closed

SimpleXMLElement regressions on Psalm 5.15 #10204

edsrzf opened this issue Sep 14, 2023 · 3 comments

Comments

@edsrzf
Copy link
Contributor

edsrzf commented Sep 14, 2023

I'm running into a couple regressions related to SimpleXMLElement when trying to upgrade to Psalm 5.15:

  • False positive RedundantCondition errors when calling isset on property fetches
  • Less specific return type for the asXml method when no arguments are passed

Example code:

<?php

function f(): string|false
{
    $x = new SimpleXMLElement('<a></a>');
    // False positive: RedundantCondition
    if (isset($x->foo)) {
        echo 'hi';
    }
    // Inferred type not specific enough
    return $x->asXml();
}

For some reason I can't reproduce either issue on psalm.dev, but I've created a repository here: https://github.com/edsrzf/psalm-xml-repro

I'm guessing these are both due to #10049.

@edsrzf
Copy link
Contributor Author

edsrzf commented Sep 16, 2023

I think the problem is that I didn't have the simplexml extension enabled, by either adding ext-simplexml as a dependency in composer.json or via enableExtensions in psalm.xml. With either of those, the errors go away.

I guess this makes sense but I find it surprising to have to make this change in a minor version update.

@robchett
Copy link
Contributor

robchett commented Oct 8, 2023

I'm not 100% sure, but I think the online tool has all extensions enabled. At the very least it appears to have simplexml. Which is why this can't be reproduced there.

Before a4de6d9 SimpleXMLElement & SimpleXMLIterator were defined as universal_object_crates, even if the extension was not explicitly enabled, and reflection was used for the signatures.

I don't think it's right to reintroduce the definitions without explicit loading of the extension, so would class this as a bugfix instead of a regression.

@edsrzf
Copy link
Contributor Author

edsrzf commented Nov 24, 2023

I agree there's probably nothing to fix here. The BC break is mentioned in the linked PR and hopefully anyone running into it will find this issue.

It might be nice to update the 5.15.0 release notes to mention this more clearly though.

@edsrzf edsrzf closed this as completed Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants