diff --git a/CHANGELOG.md b/CHANGELOG.md index 727c9f31..f21e6043 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lisp/php-complete.el b/lisp/php-complete.el index fe3050cd..394e3efb 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: @@ -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)) diff --git a/lisp/php-defs.el b/lisp/php-defs.el index 8be83138..d528e4de 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" @@ -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 diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 1d5ef11f..3696a443 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,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.