From 6a46dbe03c565cbe1b8eaac4ed99979708c49f1f Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Tue, 4 Aug 2026 22:24:28 +0900 Subject: [PATCH 1/4] Fix the :safe predicate of php-complete-function-modules The predicate had two independent defects. It looped over `values' -- a standard (Emacs 29+ obsolete) Emacs variable that is nonetheless bound, normally to nil -- instead of over its own argument `value'. `cl-loop for v in nil always ...' is vacuously true, so the predicate returned t for every list, and a .dir-locals.el could put arbitrary unvetted module names into the variable: pred '(core) -> t pred '(bogus-module) -> t ; should be nil pred '("anything" 42) -> t ; should be nil It also never ran successfully in the first place. Emacs decides whether a .dir-locals.el value is safe while hacking local variables, which happens before php-complete.el is loaded, so the predicate executes as copied into php-mode-autoloads.el -- where neither cl-lib nor php-defs-functions-alist exists yet: php-complete-function-modules -> (void-function cl-loop) safe-local-variable-p demotes that error and returns nil, so the variable was treated as unsafe and prompted for confirmation anyway. Add an autoloaded `php-defs-function-module-names' for the predicate to consult -- a plain list of module symbols, rather than autoloading the 3800-line `php-defs-functions-alist' itself -- and rewrite the predicate without cl-lib. `php-defs-function-module-names' also feeds the defcustom `:type', and a test keeps it in sync with the alist. --- CHANGELOG.md | 3 ++ lisp/php-complete.el | 16 ++++++++--- lisp/php-defs.el | 20 +++++++++++++ tests/php-mode-test.el | 65 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 727c9f31..1ec43379 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ All notable changes of the PHP Mode 1.19.1 release series are documented in this ### Fixed * `php-project-project-find-function` now returns a `project.el` value valid on Emacs 28+; it previously built a `(vc . ROOT)` cons that broke the 3-element `(vc BACKEND ROOT)` representation and made `project-root` signal an error + * Fix the `:safe` predicate of `php-complete-function-modules`, which accepted any list and never actually applied + * It looped over the standard Emacs variable `values` instead of its own argument, so `cl-loop ... always` succeeded vacuously and unknown module names passed as safe + * Emacs also checks directory-local values *before* php-complete.el is loaded, so the predicate ran as copied into the autoloads file and hit `void-function cl-loop`. `safe-local-variable-p` demotes such errors to nil, so every project setting this variable was prompted for confirmation regardless ### Deprecated diff --git a/lisp/php-complete.el b/lisp/php-complete.el index fe3050cd..13566d4a 100644 --- a/lisp/php-complete.el +++ b/lisp/php-complete.el @@ -48,10 +48,18 @@ (defcustom php-complete-function-modules '(bcmath core gmp libxml intl mbstring pcntl posix sodium xml xmlwriter) "Module names for function names completion." :tag "PHP Complete Function Modules" - :type (eval-when-compile `(set ,@(mapcar (lambda (elm) (list 'const (car elm))) - php-defs-functions-alist))) - :safe (lambda (value) (and (listp value) (cl-loop for v in values - always (assq v php-defs-functions-alist))))) + :type (eval-when-compile `(set ,@(mapcar (lambda (name) (list 'const name)) + php-defs-function-module-names))) + ;; Only accept module names PHP Mode actually knows about. Deliberately + ;; written without `cl-lib' and against the autoloaded + ;; `php-defs-function-module-names': this predicate is copied verbatim into + ;; the package autoloads file and runs there while Emacs checks + ;; .dir-locals.el, where neither cl-lib nor php-defs.el is loaded yet. + :safe (lambda (value) + (and (proper-list-p value) + (not (memq nil (mapcar (lambda (v) + (and (memq v php-defs-function-module-names) t)) + value)))))) ;;; Cape functions: diff --git a/lisp/php-defs.el b/lisp/php-defs.el index 8be83138..92e0ee3e 100644 --- a/lisp/php-defs.el +++ b/lisp/php-defs.el @@ -26,6 +26,26 @@ ;;; Code: +;; Autoloaded because the `:safe' predicate of `php-complete-function-modules' +;; consults this list, and that predicate is copied into the package autoloads +;; file, where it runs while Emacs checks .dir-locals.el — long before +;; php-defs.el itself loads. Spelled out literally rather than derived from +;; `php-defs-functions-alist' for the same reason; the two are kept in sync by +;; `php-complete-test-function-module-names-match-alist'. +;;;###autoload +(defvar php-defs-function-module-names + '(apache apcu bcmath bzip2 calendar com_dotnet commonmark componere core + cubrid curl dba dbase dio eio enchant exif expect fann fdf fpm ftp gd + gearman geoip gettext gmp gnupg ibase ibm_db2 iconv igbinary imap inotify + intl language ldap libxml lzf mailparse mbstring memcache mongodb mqseries + mysql-obsolete mysqli oauth obsolete_7 obsolete_8 oci8 odbc openal openssl + parallel pcntl pgsql posix ps pspell radius rar readline recode rpminfo rrd + runkit7 scoutapm shmop simplexml smnp soap socket sodium solr sqlsrv ssdeep + ssh2 stats stomp svn swoole sysvshm taint tcpwrap tidy trader ui uopz + var_representation win32service wincache xattr xdiff xhprof xml xmlrpc + xmlwriter yaml yaz zlib zookeeper) + "List of the module names that key `php-defs-functions-alist'.") + (defvar php-defs-functions-alist '((apache "apache_child_terminate" diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 1d5ef11f..f141444d 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -31,6 +31,8 @@ ;;; Code: (require 'php) +(require 'php-complete) +(require 'php-defs) (require 'php-mode) (require 'php-mode-debug) (require 'php-project) @@ -860,6 +862,69 @@ half-fontified: the `|' plain and the `>' as `php-comparison-op'." (with-php-mode-test ("indent/issue-227.php" :indent t :magic t :style pear)) (with-php-mode-test ("indent/issue-774.php" :indent t :magic t :style pear))) +(ert-deftest php-complete-test-function-module-names-match-alist () + "`php-defs-function-module-names' must list every module of the alist. + +The names are spelled out literally so that the autoloads copy of the +`php-complete-function-modules' `:safe' predicate can consult them +without php-defs.el being loaded; this keeps that literal honest." + (should (equal php-defs-function-module-names + (mapcar #'car php-defs-functions-alist)))) + +(ert-deftest php-complete-test-function-modules-safe-local-variable () + "Only module names known to PHP Mode are safe for .dir-locals.el. + +Regression test: the predicate looped over the standard Emacs variable +`values' instead of its own argument, so `cl-loop ... always' succeeded +vacuously and any list at all was accepted as safe." + (let ((pred (get 'php-complete-function-modules 'safe-local-variable))) + (should pred) + (should (funcall pred '(core))) + (should (funcall pred '(bcmath core pcntl))) + (should (funcall pred nil)) + (should-not (funcall pred '(bogus-module))) + (should-not (funcall pred '(core bogus-module))) + (should-not (funcall pred '("anything" 42))) + (should-not (funcall pred '(core . bcmath))))) + +(ert-deftest php-complete-test-safe-local-variable-works-from-autoloads () + "The `:safe' predicate must work from the package autoloads file alone. + +Emacs decides whether a .dir-locals.el value is safe while hacking local +variables, which happens before php-complete.el is loaded, so the +predicate runs as copied into php-mode-autoloads.el. There it must not +depend on cl-lib nor on variables that only php-defs.el defines, or +`safe-local-variable-p' demotes the resulting error to nil and every +project setting this variable gets a confirmation prompt anyway." + (let ((autoloads (expand-file-name "../lisp/php-mode-autoloads.el" php-mode-test-dir)) + (emacs (expand-file-name invocation-name invocation-directory))) + (skip-unless (file-exists-p autoloads)) + (with-temp-buffer + (let ((status (call-process + emacs nil t nil "-Q" "--batch" + "--load" autoloads + "--eval" + (prin1-to-string + '(progn + ;; Guard against the predicate quietly working only + ;; because php-complete.el got loaded after all. + (when (featurep 'php-complete) + (error "Feature php-complete must not be loaded in this check")) + (dolist (c '(((core) t) + ((bcmath core pcntl) t) + (nil t) + ((bogus-module) nil) + (("anything" 42) nil))) + (let* ((pred (get 'php-complete-function-modules + 'safe-local-variable)) + (got (and (funcall pred (nth 0 c)) t))) + (unless (eq got (nth 1 c)) + (error "Value %S: got %S, want %S" + (nth 0 c) got (nth 1 c))))) + (princ "OK")))))) + (should (eq 0 status)) + (should (string-match-p "OK" (buffer-string))))))) + ;; For developers: How to make .faces list file. ;; ;; 1. Press `M-x eval-buffer' in this file bufffer. From 00339ea8c8de56a3914d715da2bb4134b1288453 Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Tue, 4 Aug 2026 22:25:08 +0900 Subject: [PATCH 2/4] Stop php-complete--functions from sorting a user option in place `sort' is destructive, so sorting `php-complete-function-modules' to build the cache key reordered the value the user had set: after a single completion, a configured (pcntl bcmath core) came back as (bcmath core pcntl). Sort a copy instead. --- CHANGELOG.md | 2 ++ lisp/php-complete.el | 4 +++- tests/php-mode-test.el | 11 +++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ec43379..d42c5c4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ All notable changes of the PHP Mode 1.19.1 release series are documented in this * Fix the `:safe` predicate of `php-complete-function-modules`, which accepted any list and never actually applied * It looped over the standard Emacs variable `values` instead of its own argument, so `cl-loop ... always` succeeded vacuously and unknown module names passed as safe * Emacs also checks directory-local values *before* php-complete.el is loaded, so the predicate ran as copied into the autoloads file and hit `void-function cl-loop`. `safe-local-variable-p` demotes such errors to nil, so every project setting this variable was prompted for confirmation regardless + * Fix function name completion sorting the user's `php-complete-function-modules` in place + * The first completion reordered the value the user had set, e.g. `(pcntl bcmath core)` became `(bcmath core pcntl)` ### Deprecated diff --git a/lisp/php-complete.el b/lisp/php-complete.el index 13566d4a..a22ce75c 100644 --- a/lisp/php-complete.el +++ b/lisp/php-complete.el @@ -96,7 +96,9 @@ SORT should be nil to disable sorting." ;;; Data source functions: (defun php-complete--functions () "Return PHP function names." - (let* ((modules (sort php-complete-function-modules #'string<)) + ;; `sort' is destructive, so copy before sorting: `php-complete-function-modules' + ;; is a user option and must not be reordered under the user's feet. + (let* ((modules (sort (copy-sequence php-complete-function-modules) #'string<)) (functions (gethash modules php-complete--functions-cache))) (unless functions (setq functions (sort (cl-loop for module in modules diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index f141444d..0f7c3f63 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -887,6 +887,17 @@ vacuously and any list at all was accepted as safe." (should-not (funcall pred '("anything" 42))) (should-not (funcall pred '(core . bcmath))))) +(ert-deftest php-complete-test-functions-does-not-mutate-user-option () + "`php-complete--functions' must leave `php-complete-function-modules' alone. + +Regression test: it sorted the user option in place, so the first +completion silently reordered the value the user had set." + (let* ((modules '(pcntl bcmath core)) + (php-complete-function-modules (copy-sequence modules)) + (php-complete--functions-cache (make-hash-table :test #'equal))) + (php-complete--functions) + (should (equal modules php-complete-function-modules)))) + (ert-deftest php-complete-test-safe-local-variable-works-from-autoloads () "The `:safe' predicate must work from the package autoloads file alone. From 66f8538f16fd83e62f22651d25facb2b94c0644d Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Tue, 4 Aug 2026 22:25:57 +0900 Subject: [PATCH 3/4] Keep module names out of PHP function completion candidates An entry of `php-defs-functions-alist' is (MODULE . FUNCTION-NAMES), so appending the `assq' result spliced MODULE into the candidate list: with `php-complete-function-modules' set to (bcmath), the symbol `bcmath' showed up among the function name strings. Take the `cdr'. Also give `php-defs-functions-alist' the docstring it should have had as a public variable, spelling out that shape. --- CHANGELOG.md | 2 ++ lisp/php-complete.el | 5 ++++- lisp/php-defs.el | 7 ++++++- tests/php-mode-test.el | 15 +++++++++++++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d42c5c4d..8962e75c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ All notable changes of the PHP Mode 1.19.1 release series are documented in this * Emacs also checks directory-local values *before* php-complete.el is loaded, so the predicate ran as copied into the autoloads file and hit `void-function cl-loop`. `safe-local-variable-p` demotes such errors to nil, so every project setting this variable was prompted for confirmation regardless * Fix function name completion sorting the user's `php-complete-function-modules` in place * The first completion reordered the value the user had set, e.g. `(pcntl bcmath core)` became `(bcmath core pcntl)` + * Fix function name completion offering module names as if they were PHP functions + * Entries of `php-defs-functions-alist` are `(MODULE . FUNCTION-NAMES)` and the whole entry was appended, so every enabled module leaked its own name into the candidates ### Deprecated diff --git a/lisp/php-complete.el b/lisp/php-complete.el index a22ce75c..394e3efb 100644 --- a/lisp/php-complete.el +++ b/lisp/php-complete.el @@ -101,8 +101,11 @@ SORT should be nil to disable sorting." (let* ((modules (sort (copy-sequence php-complete-function-modules) #'string<)) (functions (gethash modules php-complete--functions-cache))) (unless functions + ;; Take the `cdr': an entry of `php-defs-functions-alist' is + ;; (MODULE . FUNCTION-NAMES), so appending the entry itself would + ;; splice MODULE in among the function names. (setq functions (sort (cl-loop for module in modules - append (assq module php-defs-functions-alist)) + append (cdr (assq module php-defs-functions-alist))) #'string<)) (puthash modules functions php-complete--functions-cache)) functions)) diff --git a/lisp/php-defs.el b/lisp/php-defs.el index 92e0ee3e..d528e4de 100644 --- a/lisp/php-defs.el +++ b/lisp/php-defs.el @@ -3906,7 +3906,12 @@ "zlib_encode" "zlib_get_coding_type") (zookeeper - "zookeeper_dispatch"))) + "zookeeper_dispatch")) + "Alist of PHP function names provided by each module. + +Each element is (MODULE . FUNCTION-NAMES), where MODULE is a symbol also +listed in `php-defs-function-module-names' and FUNCTION-NAMES is a list +of function name strings.") (provide 'php-defs) ;;; php-defs.el ends here diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 0f7c3f63..4cffc17e 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -887,6 +887,21 @@ vacuously and any list at all was accepted as safe." (should-not (funcall pred '("anything" 42))) (should-not (funcall pred '(core . bcmath))))) +(ert-deftest php-complete-test-functions-returns-only-function-names () + "`php-complete--functions' must return function names and nothing else. + +Regression test: it appended the whole (MODULE . FUNCTION-NAMES) entry of +`php-defs-functions-alist', so each enabled module leaked its own name +into the completion candidates as a symbol." + (let* ((php-complete-function-modules '(bcmath)) + (php-complete--functions-cache (make-hash-table :test #'equal)) + (functions (php-complete--functions))) + (should functions) + (should-not (seq-remove #'stringp functions)) + (should (member "bcadd" functions)) + (should (equal functions (sort (copy-sequence (cdr (assq 'bcmath php-defs-functions-alist))) + #'string<))))) + (ert-deftest php-complete-test-functions-does-not-mutate-user-option () "`php-complete--functions' must leave `php-complete-function-modules' alone. From dba8e18a91887a193d6af1c94a3f4bae7518df0a Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Tue, 4 Aug 2026 23:06:32 +0900 Subject: [PATCH 4/4] Skip the autoloads :safe check on Emacs versions that drop :safe Emacs 28 was the first to copy a defcustom's `:safe' predicate into the generated autoloads file (autoload.el, "Propagate the :safe property to the loaddefs file"). Emacs 27's generator converts the defcustom into a plain defvar and discards `:safe' entirely, so the autoloads file has no `safe-local-variable' property to call and the subprocess died with (void-function nil) -- the CI failure on the 27.2 jobs. Skip the test when the generated file carries no such property, and check the subprocess output before its exit status so that a real failure reports what the subprocess complained about. Note in the CHANGELOG that this half of the fix needs Emacs 28 or later. --- CHANGELOG.md | 1 + tests/php-mode-test.el | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8962e75c..f21e6043 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ All notable changes of the PHP Mode 1.19.1 release series are documented in this * Fix the `:safe` predicate of `php-complete-function-modules`, which accepted any list and never actually applied * It looped over the standard Emacs variable `values` instead of its own argument, so `cl-loop ... always` succeeded vacuously and unknown module names passed as safe * Emacs also checks directory-local values *before* php-complete.el is loaded, so the predicate ran as copied into the autoloads file and hit `void-function cl-loop`. `safe-local-variable-p` demotes such errors to nil, so every project setting this variable was prompted for confirmation regardless + * That half of the fix applies on Emacs 28 and later; Emacs 27 does not copy `:safe` predicates into the autoloads file at all, so it keeps prompting until php-complete.el is loaded * Fix function name completion sorting the user's `php-complete-function-modules` in place * The first completion reordered the value the user had set, e.g. `(pcntl bcmath core)` became `(bcmath core pcntl)` * Fix function name completion offering module names as if they were PHP functions diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 4cffc17e..3696a443 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -925,6 +925,12 @@ project setting this variable gets a confirmation prompt anyway." (let ((autoloads (expand-file-name "../lisp/php-mode-autoloads.el" php-mode-test-dir)) (emacs (expand-file-name invocation-name invocation-directory))) (skip-unless (file-exists-p autoloads)) + ;; Emacs 28 was the first to copy a defcustom's `:safe' predicate into the + ;; generated autoloads file; Emacs 27 drops it, so there is nothing to + ;; check there. + (skip-unless (with-temp-buffer + (insert-file-contents autoloads) + (search-forward "'php-complete-function-modules 'safe-local-variable" nil t))) (with-temp-buffer (let ((status (call-process emacs nil t nil "-Q" "--batch" @@ -948,8 +954,10 @@ project setting this variable gets a confirmation prompt anyway." (error "Value %S: got %S, want %S" (nth 0 c) got (nth 1 c))))) (princ "OK")))))) - (should (eq 0 status)) - (should (string-match-p "OK" (buffer-string))))))) + ;; Check the output first: on failure ERT then reports what the + ;; subprocess complained about, not just its exit status. + (should (string-match-p "OK" (buffer-string))) + (should (eq 0 status)))))) ;; For developers: How to make .faces list file. ;;