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

PHP84 : Add spec compliance #74

Merged
merged 12 commits into from
Sep 18, 2024
Merged

PHP84 : Add spec compliance #74

merged 12 commits into from
Sep 18, 2024

Conversation

veewee
Copy link
Owner

@veewee veewee commented Feb 21, 2024

Q A
Type improvement
BC Break yes
Fixed issues #72

Summary

Implements the new PHP 8.4 spec-compliance RFC:

Current stats:

 ...............................................................  63 / 607 ( 10%)
............................................................... 126 / 607 ( 20%)
............................................................... 189 / 607 ( 31%)
............................................................... 252 / 607 ( 41%)
............................................................... 315 / 607 ( 51%)
............................................................... 378 / 607 ( 62%)
............................................................... 441 / 607 ( 72%)
............................................................... 504 / 607 ( 83%)
............................................................... 567 / 607 ( 93%)
........................................                        607 / 607 (100%)

Time: 00:00.183, Memory: 34.43 MB

OK (607 tests, 1047 assertions)

Target PHP version: 8.1 (set by config file) Enabled extensions: dom.
Scanning files...
Analyzing files...

░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░  60 / 383 (15%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 120 / 383 (31%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 180 / 383 (46%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 240 / 383 (62%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 300 / 383 (78%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 360 / 383 (93%)
░░░░░░░░░░░░░░░░░░░░░░░
------------------------------

       No errors found!

------------------------------

Checks took 2.46 seconds and used 87.322MB of memory
Psalm was able to infer types for 100% of the codebase
E.............E...................................   ( 50 / 304)
..................................................   (100 / 304)
...............................................EEE   (150 / 304)
..................................................   (200 / 304)
................................................T.   (250 / 304)
............................E....E................   (300 / 304)
....                                                 (304 / 304)

304 mutations were generated:
     296 mutants were killed
       0 mutants were configured to be ignored
       0 mutants were not covered by tests
       0 covered mutants were not detected
       7 errors were encountered
       0 syntax errors were encountered
       1 time outs were encountered
       0 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 100%
         Mutation Code Coverage: 100%
         Covered Code MSI: 100%

Checklist:

  • Upgrade to new DOM spec compliance implementation.
  • Unit tests
  • Psalm
  • Cs-fixer
  • Infection
  • Update documentation for changed functions / added functions.
    • Review : Does the new API makes sense... ?
  • Remove open TODOs

@veewee veewee marked this pull request as draft February 21, 2024 15:08
@veewee veewee force-pushed the php84-spec-compliance branch 13 times, most recently from 2a0559f to 74328c7 Compare February 23, 2024 10:13
src/Xml/Dom/Configurator/pretty_print.php Outdated Show resolved Hide resolved
src/Xml/Dom/Configurator/canonicalize.php Show resolved Hide resolved
src/Xml/Dom/Locator/Attribute/xmlns_attributes_list.php Outdated Show resolved Hide resolved
src/Xml/Dom/Locator/Node/value.php Outdated Show resolved Hide resolved

interface Loader
{
public function __invoke(DOMDocument $document): void;
public function __invoke(): XMLDocument;
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO : document new API for loading DOM

@veewee veewee force-pushed the php84-spec-compliance branch from 8a61a7b to 65810ed Compare February 23, 2024 15:07
@veewee veewee force-pushed the php84-spec-compliance branch from 65810ed to c5c3c5b Compare February 24, 2024 17:52
@@ -8,6 +8,8 @@
use XSLTProcessor;

/**
* TODO : Add support for callables : https://wiki.php.net/rfc/improve_callbacks_dom_and_xsl (either here or through a separate configurator)
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO : Add support

@nielsdos
Copy link

nielsdos commented Mar 3, 2024

Just letting you know that while waiting for the reviews, I spent some time today implementing the missing APIs in a separate branch. (Can be found at nielsdos/php-src#93)
In particular:

  • DOM\Element::$substitutedNodeValue (I might add the same one for DOM\Attr too, not sure yet)
  • DOM\Element::getInScopeNamespaces(): array
  • DOM\Element::rename(?string $namespaceURI, string $qualifiedName): void and also for DOM\Attr.

Fortunately, as expected, these were rather easy to implement.

@veewee
Copy link
Owner Author

veewee commented Mar 4, 2024

Cool! I'm a bit busy this week but will try to play around with it soon :)

@veewee veewee force-pushed the php84-spec-compliance branch 2 times, most recently from 26d0875 to 644dd7d Compare March 10, 2024 08:09
@veewee
Copy link
Owner Author

veewee commented Mar 10, 2024

Just wanted to give you a small update @nielsdos :

  • The new entity substitution works flawlessly
  • Renaming nodes works in general, but I have a feeling there are a few cases not working properly (will come back to this when later, I now covered about half of the test-suite but I need to dive into the specifics so that I can give you more detailed information)
  • Removing namespaces works as described above (one case still failing - need to dive into the specifics as well) - It's getting a bit/log more complex now though.
  • The getInScopeNamespaces works theoretically, but I expect it to work a bit different. I've mentioned in your PR : Extra dom additions 1 nielsdos/php-src#93 (comment) (this results in the optimization code throwing 21 failures right now)

Tests: 608, Assertions: 1022, Errors: 20, Failures: 21.

Getting closer to the end goal :)

@nielsdos
Copy link

Thanks for the update! Nice to see ^^

I'll try to have a look soon-ish at how to deal with the getInScopeNamespaces() discrepancy.

As for the other issues: if you're stuck debugging them, I could always try to have a look at your code with a debug PHP build, if I know what test case to look for :)

@veewee veewee force-pushed the php84-spec-compliance branch 2 times, most recently from 334e098 to a1af17d Compare March 14, 2024 06:08
@veewee veewee force-pushed the php84-spec-compliance branch 4 times, most recently from e25f849 to 18ab1a9 Compare July 9, 2024 12:37
@veewee veewee force-pushed the php84-spec-compliance branch 5 times, most recently from dc3ab08 to f42b46e Compare September 18, 2024 06:32
@veewee veewee force-pushed the php84-spec-compliance branch from f42b46e to 672e71a Compare September 18, 2024 13:40
@veewee veewee changed the title [WIP] PHP84 : Add spec compliance PHP84 : Add spec compliance Sep 18, 2024
@veewee veewee marked this pull request as ready for review September 18, 2024 14:04
@veewee veewee changed the base branch from 3.x to 4.x September 18, 2024 14:05
@veewee veewee added enhancement New feature or request Breaking change labels Sep 18, 2024
@veewee veewee merged commit cebfafb into 4.x Sep 18, 2024
10 checks passed
@veewee veewee deleted the php84-spec-compliance branch September 20, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants