Fix three bugs in php-complete function name completion

CHANGELOG.md

@@ -17,6 +17,14 @@ 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
+   * 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
+   * 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
 

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:
 
@@ -88,11 +96,16 @@ 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
+      ;; 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))

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"
@@ -3886,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

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,103 @@ 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-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.
+
+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.
+
+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))
+    ;; 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"
+                     "--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"))))))
+        ;; 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.
 ;;
 ;; 1. Press `M-x eval-buffer' in this file bufffer.