-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/readline: readline_info fix usage when the buffer is not initialised #15139
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
Conversation
bea1cf3 to
3ab4ff7
Compare
Is this perhaps also related to this issue:
./configure --with-libedit make /php-src/ext/readline/readline.c: In function ‘zif_readline_info’: /php-src/ext/readline/readline.c:191:41: warning: implicit declaration of function ‘rl_extend_line_buffer’ [-Wimplicit-function-declaration] 191 | rl_extend_line_buffer(Z_STRLEN_P(value) + 1); | ^~~~~~~~~~~~~~~~~~~~~ /usr/bin/ld: ext/readline/readline.o: in function `zif_readline_info': /php-src/ext/readline/readline.c:191:(.text+0x747): undefined reference to `rl_extend_line_buffer'
The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).
Not at all related but was not aware of the libedit/libreadline differences.
ext/readline/readline.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused, surely having the if (!rl_line_buffer) check first makes more sense? As you then don't need to check for !oldstr ?
3ab4ff7 to
3eb00aa
Compare
if the new value was larger, rl_line_buffer_length was never updated. neither was rl_end (on unixes). close GH-15120
The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).
Indeed, Licenses are NOT compatible, IMHO at some point we should drop libreadline support
The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).
Indeed, Licenses are NOT compatible, IMHO at some point we should drop libreadline support
Let s try it for next major release then (php 9 ?).
Quick check,
If seems rl_end is not updated on Linux with libedit
#if !defined(PHP_WIN32) && !HAVE_LIBEDIT
...
rl_end = Z_STRLEN_P(value);
But may be returned
#ifndef PHP_WIN32
} else if (zend_string_equals_literal_ci(what, "end")) {
RETVAL_LONG(rl_end);
#endif
rl_initialise is only called when readline() is used so the global buffer might not be initialised yet.
ff2b44f to
56ec48a
Compare
Faced the same trying to build 8.4.0_alpha3
Thank you, the PR allowed to pass tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It works, thanks!
Will it cause the alpha3 tarball rebuild or better to wait for beta1?
Will it cause the alpha3 tarball rebuild or better to wait for beta1?
neither, there is alpha4.
rl_initialise is only called when readline() is used so the global
buffer might not be initialised yet.