Skip to content

Create separate lexbor extension #18538

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Create separate lexbor extension #18538

wants to merge 8 commits into from
+303,207 −147,746

Conversation

kocsismate
Copy link
Member

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.

@kocsismate kocsismate requested review from TimWolla and petk May 11, 2025 21:29
@kocsismate kocsismate requested a review from nielsdos as a code owner May 11, 2025 21:29
Girgias
Girgias reviewed May 12, 2025
View reviewed changes
@kocsismate kocsismate force-pushed the lexbor-extension branch 3 times, most recently from c228b4a to accf126 Compare May 13, 2025 04:45
@TimWolla
Copy link
Member

@cmb69 for Windows maybe?

@cmb69
Copy link
Member

cmb69 commented May 13, 2025

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.

@nielsdos
Copy link
Member

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
Copy link
Member Author

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
TimWolla reviewed May 13, 2025
View reviewed changes
ext/lexbor/config.w32 Outdated
@@ -26,5 +26,5 @@ if (PHP_LEXBOR == "yes") {

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 /");
Copy link
Member

Choose a reason for hiding this comment

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

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/");

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@nielsdos
Copy link
Member

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!

@kocsismate kocsismate force-pushed the lexbor-extension branch 5 times, most recently from c938572 to 78d185b Compare May 15, 2025 07:07
kocsismate
kocsismate commented May 15, 2025
View reviewed changes
ext/dom/config.w32
@@ -16,27 +16,12 @@ if (PHP_DOM == "yes") {
entityreference.c \
token_list.c \
notation.c xpath.c dom_iterators.c \
namednodemap.c xpath_callbacks.c", null, "-Iext/dom/lexbor");
namednodemap.c xpath_callbacks.c", null, "/I ext/lexbor");
Copy link
Member Author

Choose a reason for hiding this comment

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

@cmb69 Currently, the build fails with the following error:

ext\dom\parentnode\css_selectors.c(26): fatal error C1083: Cannot open include file: 'lexbor/selectors-adapted/selectors.h': No such file or directory

I installed the relevant header file below, but do you think I should add something like -Iext/dom/ here? Please note that previously, the file was in ext/dom/lexbor/lexbor/lexbor-adapted/selectors.h, and now I removed one level from the path.

Copy link
Member Author

Choose a reason for hiding this comment

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

P.S. I wanted to mean -Iext/lexbor/ instead of -Iext/dom/

@nielsdos nielsdos force-pushed the lexbor-extension branch from 6d631e1 to a5072e2 Compare May 16, 2025 19:28
@nielsdos
Copy link
Member

nielsdos commented May 16, 2025 •
edited
Loading

TimWolla
TimWolla reviewed May 16, 2025
View reviewed changes
ext/lexbor/php_lexbor.c
static PHP_MINFO_FUNCTION(lexbor)
{
php_info_print_table_start();
php_info_print_table_row(2, "Lexbor support", "active");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@kocsismate
Copy link
Member Author

Is it possible to merge this before the URI API vote ends? Doing so would make it a tiny little bit easier for me to rebase #14461.

@TimWolla
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimWolla TimWolla TimWolla left review comments

@Girgias Girgias Girgias left review comments

@petk petk Awaiting requested review from petk

@nielsdos nielsdos Awaiting requested review from nielsdos nielsdos is a code owner

Assignees
No one assigned
Labels
Category: Build System Extension: dom
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kocsismate @TimWolla @cmb69 @nielsdos @Girgias