add readline_list_history with libedit >= 3.1

[remicollet]

@weltling can you please check change needed for windows
(can't find libedit in win32/build/libs_version.txt)

[weltling]

@remicollet on Windows it's a surrogate API compatible library https://sourceforge.net/projects/mingweditline/. I'm not sure, to which exact version of libedit it corresponds. From what i was just checking, it misses history_get_history_state as in your patch. If that symbol could be worked around somhow, it probably should work. Probably I'd also check to extend the implementation of WinEditLine otherwise.

Thansk.

[cmb69]

The following quick-and-dirty patch applied on this PR seems to work:

 ext/readline/config.w32 |  1 +
 ext/readline/readline.c | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/ext/readline/config.w32 b/ext/readline/config.w32
index 9e21e6bb54..640f2e3107 100644
--- a/ext/readline/config.w32
+++ b/ext/readline/config.w32
@@ -9,6 +9,7 @@ if (PHP_READLINE != "no") {
 		EXTENSION("readline", "readline.c readline_cli.c");
 		ADD_FLAG("CFLAGS_READLINE", "/D HAVE_LIBEDIT");
 		ADD_FLAG("CFLAGS_READLINE", "/D HAVE_RL_COMPLETION_MATCHES");
+		ADD_FLAG("CFLAGS_READLINE", "/D HAVE_HISTORY_LIST");
 	} else {
 		WARNING("readline not enabled; libraries and headers not found");
 	}
diff --git a/ext/readline/readline.c b/ext/readline/readline.c
index 64c7349b62..4099c30264 100644
--- a/ext/readline/readline.c
+++ b/ext/readline/readline.c
@@ -352,7 +352,7 @@ PHP_FUNCTION(readline_add_history)
 		return;
 	}
 
-	add_history(arg);
+	char * res= add_history(arg);
 
 	RETURN_TRUE;
 }
@@ -384,7 +384,7 @@ PHP_FUNCTION(readline_list_history)
 {
 	HIST_ENTRY **history;
 #ifdef HAVE_LIBEDIT
-	HISTORY_STATE *hs;
+	// HISTORY_STATE *hs;
 #endif
 
 	if (zend_parse_parameters_none() == FAILURE) {
@@ -394,13 +394,13 @@ PHP_FUNCTION(readline_list_history)
 	array_init(return_value);
 
 #ifdef HAVE_LIBEDIT
-	using_history();
+	// using_history();
 	history = history_list();
-	hs = history_get_history_state();
+	// hs = history_get_history_state();
 
-	if (history && hs) {
-		int i;
-		for (i = 0; i < hs->length; i++) {
+	if (history) {
+		int i, n = history_length();
+		for (i = 0; i < n; i++) {
 #else
 	history = history_list();

readline_read_history_001.phpt fails due to the tempnam("/tmp", …) which would have to be rewritten.

It's likely best to have the Windows implementation completely separate, i.e. using a single #ifdef PHP_WIN32 … #endif.

I'm confused that I had to comment out using_history(), since it appears to clear the history on Windows, apparently quite contrary to the behavior on Linux. It should be investigated whether it should be called in MINIT on Windows (like it's done for libreadline).

[remicollet]

Patch updated to include windows changes

[cmb69]

Suggested patch for the PHPT which fails on Windows:

 ext/readline/tests/readline_read_history_001.phpt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/readline/tests/readline_read_history_001.phpt b/ext/readline/tests/readline_read_history_001.phpt
index fcdb1ae325..cf60532611 100644
--- a/ext/readline/tests/readline_read_history_001.phpt
+++ b/ext/readline/tests/readline_read_history_001.phpt
@@ -5,7 +5,7 @@ readline_read_history(): Basic test
 --FILE--
 <?php
 
-$name = tempnam('/tmp', 'readline.tmp');
+$name = tempnam(sys_get_temp_dir(), 'readline.tmp');
 
 readline_add_history("foo\n");

While the patch appears to work, I wonder why you've targeted PHP-7.2, Remi. Is there any urgent need to make readline_list_history() available?

[remicollet]

Is there any urgent need to make readline_list_history() available?

Not really urgent, just trying to reduce gap between platforms.

BTW, readline_read_history_001.phpt also fails on linux... because read_history seems broken in some versions... https://bugzilla.redhat.com/show_bug.cgi?id=1677331

Probably going to apply only in 7.4+

[remicollet]

Merged in 7.4