Create separate lexbor extension by kocsismate · Pull Request #18538 · php/php-src

kocsismate · 2025-05-11T21:29:10Z

In preparation of #14461 (https://wiki.php.net/rfc/url_parsing_api)

Windows support is pretty much TBD, as I have no way to test it.

EXTENSIONS

TimWolla · 2025-05-13T11:47:52Z

@cmb69 for Windows maybe?

cmb69 · 2025-05-13T12:19:56Z

I haven't actually checked that (can do later if necessary), but I think we need to install the actual lexbor headers; cf.

php-src/ext/pcre/config.w32

Line 12 in 4122daa

PHP_INSTALL_HEADERS("ext/pcre", "php_pcre.h pcre2lib/");

I think that also needs to be done for POSIX systems, so phpize builds can work.

ndossche · 2025-05-13T17:50:55Z

Definitely.

Also, you need to keep the selectors-adapted directory in ext/dom. Due to technical reasons this is a port of the upstream sources adapted to libxml2, so it has a dependency on libxml.

kocsismate · 2025-05-13T20:18:30Z

I think we need to install the actual lexbor headers; cf.

I tried to follow what ext/dom did when it used to have the lexbor sources:

https://github.com/php/php-src/blob/master/ext/dom/config.w32#L19

it didn't install the lexbor headers, other than putting them on the include path. Nevertheless, I'm trying out the suggestion.

Also, you need to keep the selectors-adapted directory in ext/dom.

Did I move it accidentally? 🤔 I tried to keep it in place, because I noticed that it was a PHP specific adaptation :)

TimWolla · 2025-05-13T20:54:27Z

ext/lexbor/config.w32

    AC_DEFINE("HAVE_LEXBOR", 1, "Define to 1 if the PHP extension 'lexbor' is available.");

-    PHP_INSTALL_HEADERS("ext/lexbor", "php_lexbor.h");
+    PHP_INSTALL_HEADERS("ext/lexbor", "php_lexbor.h /");


Windows is still broken, based on the pcre one that cmb linked, perhaps it must be this?

PHP_INSTALL_HEADERS("ext/lexbor", "php_lexbor.h lexbor/");

First, I tried to use exactly lexbor/, and then I force pushed the current attempt, but they both had the same failures.

ndossche · 2025-05-13T21:51:27Z

Did I move it accidentally? 🤔 I tried to keep it in place, because I noticed that it was a PHP specific adaptation :)

Ah sorry I saw it was moved but didn't properly check it is indeed still in dom, thanks!

ext/dom/config.w32

ndossche · 2025-05-16T19:50:09Z

I updated Lexbor as requested
Windows build is fixed, test now running in CI
Broken builds will be fixed: New compile warnings in Lexbor 2.5.0 lexbor/lexbor#278
I had to rebase on master for a conflict

TimWolla · 2025-05-16T20:50:30Z

ext/lexbor/php_lexbor.c

+static PHP_MINFO_FUNCTION(lexbor)
+{
+	php_info_print_table_start();
+	php_info_print_table_row(2, "Lexbor support", "active");


Would it make sense / would it be possible to print the Lexbor version here?

I was considering this, but Lexbor has a main version (2.5.0 is the one currently in development), and seperate versions for each component, so I wasn't sure what we want. @nielsdos Do you have any preference for exposing the version information for Lexbor?

No real preference. Exposing the main lexbor version is fine by me.

I implemented a quick and dirty solution to expose the information, since it changes only ~ once per year, so my assumption is that it's not going to be difficult to maintain the version number in the makefiles.

We could also list all of them. PHPInfo is for human rather than programmatic consumption.

As I see the question, it doesn't really matter if the granular version information is also exposed. At least I'm totally fine with the current phpinfo content.

In preparation of php#14461 (https://wiki.php.net/rfc/url_parsing_api)

Co-authored-by: Gina Peter Banyard <girgias@php.net>

TimWolla · 2025-05-19T06:57:13Z

Is it possible to merge this before the URI API vote ends?

I think that should be fine. The vote is very likely to be accepted and this is effectively an internal-only change (except for the fact that users will see an additional extension). In the worst case we revert if the vote fails.

kocsismate · 2025-05-22T08:43:12Z

@petk @cmb69 @nielsdos Could you please also review the change?

ndossche

I found the following remaining issues:

codecov.yml needs updates because of the move of lexbor sources
README.REDIST.BINS needs updates because of the move of lexbor sources (the path in the list needs updates)
Compiling Lexbor as shared extension but DOM as static fails linking: ./configure --enable-debug --disable-all --enable-lexbor=shared --with-libxml --enable-dom. This makes sense, this configuration should not be allowed
The inverse (./configure --enable-debug --disable-all --enable-lexbor --with-libxml --enable-dom=shared) also fails but should be able to work. It fails on loading the DOM extension. I think LXB_API should be set in such a way that the symbols become public

TimWolla · 2025-05-22T17:50:16Z

3. Compiling Lexbor as shared extension but DOM as static fails linking:

Since ext/uri will become a mandatory extension, ext/lexbor must as well. So this case should not even be possible.

kocsismate · 2025-05-23T20:12:34Z

Since ext/uri will become a mandatory extension, ext/lexbor must as well. So this case should not even be possible.

Yes, you are right: I realized that lexbor must have been required when I was rebasing the other PR on this one. So that one already has the fix.

ndossche

Dynamically linking against Lexbor still fails, to get that to work you need to drop -DLEXBOR_STATIC from the config

kocsismate · 2025-05-24T20:21:34Z

Dynamically linking against Lexbor still fails, to get that to work you need to drop -DLEXBOR_STATIC from the config

It turned out that I had to keep it for Windows, because there were some " inconsistent dll linkage" errors when I removed it. Unfortunately, I don't know the reason why it's different than on Unix systems, but hopefully the PR is correct now.

ndossche · 2025-05-24T20:53:23Z

It turned out that I had to keep it for Windows, because there were some " inconsistent dll linkage" errors when I removed it. Unfortunately, I don't know the reason why it's different than on Unix systems, but hopefully the PR is correct now.

Sus, because then you'll get the same issue on Windows. I'll take a look
EDIT: indeed, configure --enable-debug --disable-all --with-libxml --with-dom=shared --enable-cli fails on win32

…lexbor if dom is built statically

ndossche · 2025-05-24T22:35:28Z

It should all work now. See the commits.
I also noticed we have a duplicated reflection test that we should get rid of, but I'll wait until this PR is merged so we don't create new conflicts.

kocsismate · 2025-05-25T12:12:58Z

It should all work now. See the commits.

Thank you for all the help!

ndossche · 2025-05-25T12:25:21Z

Dropped the duplicated test in c7db07e

kocsismate requested review from TimWolla and petk May 11, 2025 21:29

kocsismate requested a review from ndossche as a code owner May 11, 2025 21:29

github-actions bot added Category: Build System Extension: dom labels May 11, 2025

kocsismate force-pushed the lexbor-extension branch from c2b1e8d to 6a309fe Compare May 11, 2025 21:36

Girgias reviewed May 12, 2025

View reviewed changes

EXTENSIONS Outdated Show resolved Hide resolved

kocsismate force-pushed the lexbor-extension branch 3 times, most recently from c228b4a to accf126 Compare May 13, 2025 04:45

kocsismate force-pushed the lexbor-extension branch from 1f92120 to 054161b Compare May 13, 2025 20:22

TimWolla reviewed May 13, 2025

View reviewed changes

kocsismate force-pushed the lexbor-extension branch 5 times, most recently from c938572 to 78d185b Compare May 15, 2025 07:07

kocsismate commented May 15, 2025

View reviewed changes

ext/dom/config.w32 Show resolved Hide resolved

ndossche force-pushed the lexbor-extension branch from 6d631e1 to a5072e2 Compare May 16, 2025 19:28

TimWolla reviewed May 16, 2025

View reviewed changes

kocsismate and others added 4 commits May 17, 2025 15:35

Create separate lexbor extension

c43c3a4

In preparation of php#14461 (https://wiki.php.net/rfc/url_parsing_api)

Update EXTENSIONS

027ad79

Co-authored-by: Gina Peter Banyard <girgias@php.net>

Trying to fix Windows builds

8dd5894

Upgrade Lexbor

9fb1d4c

ndossche requested changes May 22, 2025

View reviewed changes

Additional fixes

08739ce

kocsismate force-pushed the lexbor-extension branch from 933665a to 08739ce Compare May 23, 2025 20:39

ndossche requested changes May 24, 2025

View reviewed changes

Remove LEXBOR_STATIC option

ca304ab

kocsismate force-pushed the lexbor-extension branch from 4a8f9bd to ca304ab Compare May 24, 2025 19:31

ndossche added 9 commits May 24, 2025 23:14

Fix double definition warning

db23e9c

Add module dep

c05c8db

Drop upgrade remnant

1f43056

Fix Windows linking (maybe)

7d135d9

Resolve (non-fatal) linker warning by statically linking dom against …

851ca31

…lexbor if dom is built statically

Remove obsolete part of patch

9a540d3

Include config from build dir

04991a6

Move lexbor memory setup to ext/lexbor

b589d49

Fix reflection tests

bdd887f

ndossche requested a review from DanielEScherzer as a code owner May 24, 2025 22:01

github-actions bot added the Extension: reflection label May 24, 2025

ndossche approved these changes May 24, 2025

View reviewed changes

Add upgrading note

13272e8

kocsismate merged commit 7f59fcc into php:master May 25, 2025
7 of 9 checks passed

kocsismate deleted the lexbor-extension branch May 25, 2025 12:12

Conversation

kocsismate commented May 11, 2025

Uh oh!

Uh oh!

TimWolla commented May 13, 2025

Uh oh!

cmb69 commented May 13, 2025

Uh oh!

ndossche commented May 13, 2025

Uh oh!

kocsismate commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndossche commented May 13, 2025

Uh oh!

Uh oh!

ndossche commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimWolla commented May 19, 2025

Uh oh!

kocsismate commented May 22, 2025

Uh oh!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

TimWolla commented May 22, 2025

Uh oh!

kocsismate commented May 23, 2025

Uh oh!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

kocsismate commented May 24, 2025

Uh oh!

ndossche commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndossche commented May 24, 2025

Uh oh!

Uh oh!

kocsismate commented May 25, 2025

Uh oh!

ndossche commented May 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

ndossche commented May 16, 2025 •

edited

Loading

ndossche commented May 24, 2025 •

edited

Loading