From 531f3afe8547663118515695d3e4cfe99e1c662d Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Wed, 5 Aug 2026 20:20:27 +0900 Subject: [PATCH 1/6] Track what php-ide-mode activated instead of re-reading php-ide-features php-ide-mode decided what to deactivate by re-reading php-ide-features, so it could not keep the buffer and the mode line in agreement: - A feature that failed to activate left the mode on. `define-minor-mode' sets the mode variable before running the body, and the body signalled without undoing it, so the mode line claimed PHP-IDE was running while nothing had started -- and turning it back off then called :deactivate on an unavailable feature, signalling void-function. - Editing .dir-locals.el and re-applying it changes php-ide-features in a live buffer, after which deactivation walked the new value: it either signalled on an unknown feature or turned off the wrong one, leaving the feature that was really running behind. - Re-enabling the mode activated everything a second time, which happens whenever hack-local-variables-hook runs again, e.g. on revert-buffer. Record the features actually activated in a buffer-local variable and deactivate exactly those, skip ones already active, and roll back (mode off, features deactivated) when activation fails partway. --- lisp/php-ide.el | 62 +++++++++++++++++++++---- tests/php-mode-test.el | 101 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 10 deletions(-) diff --git a/lisp/php-ide.el b/lisp/php-ide.el index aa004bd6..0d0064a2 100644 --- a/lisp/php-ide.el +++ b/lisp/php-ide.el @@ -263,27 +263,69 @@ ACTIVATE: T is given when activating, NIL when deactivating PHP-IDE." ;; through Emacs's normal unsafe-variable confirmation, never apply silently. ) +(defvar-local php-ide--activated-features nil + "PHP-IDE features currently activated in this buffer. + +Deactivation walks this list rather than `php-ide-features', so that a +feature is always turned off through the same implementation that turned +it on, even if `php-ide-features' has changed in between --- as it does +when .dir-locals.el is edited and re-applied to a live buffer.") + ;;;###autoload (define-minor-mode php-ide-mode "Minor mode for integrate IDE-like tools." :lighter php-ide-mode-lighter + (if php-ide-mode + (php-ide--activate-features) + (php-ide--deactivate-features))) + +(defun php-ide--activate-features () + "Activate every feature named by `php-ide-features' in this buffer. + +Features already activated in this buffer are left alone, so re-enabling +the mode --- as happens whenever `hack-local-variables-hook' runs again, +for instance after `revert-buffer' --- does not activate them twice. +Removing a feature from `php-ide-features' does not deactivate it, +though; toggle `php-ide-mode' off and on, or use `php-ide-set-feature', +to apply that. + +Signals `user-error' without leaving anything half-activated: features +already turned on by this call are rolled back and `php-ide-mode' is +switched off again, so the mode line never claims PHP-IDE is running +when it is not." (let ((ide-features (if (listp php-ide-features) php-ide-features (list php-ide-features)))) (when-let* ((unavailable-features (cl-loop for feature in ide-features unless (assq feature php-ide-feature-alist) collect feature))) + (setq php-ide-mode nil) (user-error "%s includes unavailable PHP-IDE features. (available features are: %s)" - ide-features + unavailable-features (mapconcat (lambda (feature) (concat "'" (symbol-name feature))) (php-ide--available-features) ", "))) - ;; Every feature in IDE-FEATURES is guaranteed to be in `php-ide-feature-alist' here, - ;; because the loop above already signals a `user-error' otherwise. - (cl-loop for feature in ide-features - for ide-plist = (cdr (assq feature php-ide-feature-alist)) - do (progn - (run-hook-with-args 'php-ide-mode-functions feature php-ide-mode) - (if php-ide-mode - (php-ide--activate-buffer feature ide-plist) - (php-ide--deactivate-buffer ide-plist)))))) + (condition-case err + ;; Every feature in IDE-FEATURES is in `php-ide-feature-alist' here, + ;; because the loop above already signalled a `user-error' otherwise. + (dolist (feature ide-features) + (unless (memq feature php-ide--activated-features) + (let ((ide-plist (cdr (assq feature php-ide-feature-alist)))) + (run-hook-with-args 'php-ide-mode-functions feature t) + (php-ide--activate-buffer feature ide-plist) + (push feature php-ide--activated-features)))) + (error + (php-ide--deactivate-features) + (setq php-ide-mode nil) + (signal (car err) (cdr err)))))) + +(defun php-ide--deactivate-features () + "Deactivate the features this buffer actually has activated." + (dolist (feature php-ide--activated-features) + (let ((ide-plist (cdr (assq feature php-ide-feature-alist)))) + (run-hook-with-args 'php-ide-mode-functions feature nil) + ;; IDE-PLIST is non-nil for anything we activated, but a feature can be + ;; removed from `php-ide-feature-alist' while a buffer still uses it. + (when ide-plist + (php-ide--deactivate-buffer ide-plist)))) + (setq php-ide--activated-features nil)) ;;;###autoload (defun php-ide-turn-on () diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 824f1896..3a34d2ab 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -980,6 +980,107 @@ project setting this variable gets a confirmation prompt anyway." "language-server") (php-ide-eglot-server-program))))) +(defun php-mode-test--php-ide-stub-alist (log) + "Return a `php-ide-feature-alist' of test doubles recording into LOG. +LOG is a symbol whose value is a list, appended to in call order." + (list (list 'stub-ok :test (lambda () t) + :activate (lambda () (push 'activated-ok (symbol-value log))) + :deactivate (lambda () (push 'deactivated-ok (symbol-value log)))) + (list 'stub-boom :test (lambda () t) + :activate (lambda () (error "Stub feature failed to start")) + :deactivate (lambda () (push 'deactivated-boom (symbol-value log)))) + (list 'stub-unavailable :test (lambda () nil) + :activate (lambda () (push 'activated-unavailable (symbol-value log))) + :deactivate (lambda () (push 'deactivated-unavailable (symbol-value log)))))) + +(ert-deftest php-ide-test-failed-activation-leaves-mode-off () + "Regression test: a feature that cannot be activated must leave +`php-ide-mode' off. + +`define-minor-mode' sets the mode variable before running the body, and +the body used to signal without undoing that, so the mode line claimed +PHP-IDE was running while nothing had been activated -- and turning it +back off then called `:deactivate' on an unavailable feature, which +signalled `void-function' and left the user stuck." + (defvar php-mode-test--ide-log) + (let ((php-mode-test--ide-log nil)) + (with-temp-buffer + (php-mode) + (let ((php-ide-feature-alist + (php-mode-test--php-ide-stub-alist 'php-mode-test--ide-log))) + (setq-local php-ide-features '(stub-unavailable)) + (should-error (php-ide-mode +1) :type 'user-error) + (should-not php-ide-mode) + (should-not php-ide--activated-features) + ;; Nothing ran, and turning the mode off again must stay quiet. + (should-not php-mode-test--ide-log) + (php-ide-mode -1) + (should-not php-ide-mode))))) + +(ert-deftest php-ide-test-activation-rolls-back-on-error () + "A feature failing mid-list must not leave earlier ones activated." + (defvar php-mode-test--ide-log) + (let ((php-mode-test--ide-log nil)) + (with-temp-buffer + (php-mode) + (let ((php-ide-feature-alist + (php-mode-test--php-ide-stub-alist 'php-mode-test--ide-log))) + (setq-local php-ide-features '(stub-ok stub-boom)) + (should-error (php-ide-mode +1)) + (should-not php-ide-mode) + (should-not php-ide--activated-features) + ;; The one that did start must have been rolled back. + (should (equal '(activated-ok deactivated-ok) + (reverse php-mode-test--ide-log))))))) + +(ert-deftest php-ide-test-deactivates-what-it-activated () + "Regression test: deactivation must follow what was actually activated, +not the current value of `php-ide-features'. + +Editing .dir-locals.el and re-applying it changes `php-ide-features' in a +live buffer; deactivation used to walk that new value, so it either +signalled on an unknown feature or turned off the wrong one, stranding +the feature that was really running." + (defvar php-mode-test--ide-log) + (let ((php-mode-test--ide-log nil)) + (with-temp-buffer + (php-mode) + (let ((php-ide-feature-alist + (php-mode-test--php-ide-stub-alist 'php-mode-test--ide-log))) + (setq-local php-ide-features '(stub-ok)) + (php-ide-mode +1) + (should (equal '(stub-ok) php-ide--activated-features)) + ;; The project's configuration changes underneath the live buffer. + (setq-local php-ide-features '(totally-unknown-feature)) + (php-ide-mode -1) + (should-not php-ide-mode) + (should-not php-ide--activated-features) + (should (equal '(activated-ok deactivated-ok) + (reverse php-mode-test--ide-log))))))) + +(ert-deftest php-ide-test-activation-is-idempotent () + "Re-enabling `php-ide-mode' must not activate a feature twice. + +`hack-local-variables-hook' -- where the documented recipe puts +`php-ide-turn-on' -- runs again on `revert-buffer' and friends, and +`define-minor-mode' re-runs the body even when the mode is already on." + (defvar php-mode-test--ide-log) + (let ((php-mode-test--ide-log nil)) + (with-temp-buffer + (php-mode) + (let ((php-ide-feature-alist + (php-mode-test--php-ide-stub-alist 'php-mode-test--ide-log))) + (setq-local php-ide-features '(stub-ok)) + (php-ide-turn-on) + (php-ide-turn-on) + (php-ide-turn-on) + (should (equal '(stub-ok) php-ide--activated-features)) + (should (equal '(activated-ok) (reverse php-mode-test--ide-log))) + ;; And it must still deactivate exactly once. + (php-ide-mode -1) + (should (equal '(activated-ok deactivated-ok) + (reverse php-mode-test--ide-log))))))) + (ert-deftest php-ide-test-feature-alist-arity () "Regression test: `:test' must always be a callable 0-arg predicate, and for every PHP-IDE feature actually available in this Emacs, `:activate' From 907a92251bae247375f4553a9fddb0623a403549 Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Wed, 5 Aug 2026 20:20:51 +0900 Subject: [PATCH 2/6] Load php-ide-phpactor from the phpactor feature that needs it php-ide.el requires php-ide-phpactor only at compile time, yet the phpactor entry of php-ide-feature-alist names php-ide-phpactor-activate and -deactivate as its :activate/:deactivate. Those symbols resolved only because the package autoloads happened to define them; loading php-ide.el on its own, as a manual installation does, left the feature usable by :test but void-function on activation. Require it from :test, next to the phpactor package it already loads. --- lisp/php-ide.el | 7 ++++++- tests/php-mode-test.el | 13 +++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lisp/php-ide.el b/lisp/php-ide.el index 0d0064a2..62655296 100644 --- a/lisp/php-ide.el +++ b/lisp/php-ide.el @@ -114,7 +114,12 @@ '((none :test (lambda () t) :activate (lambda () t) :deactivate (lambda () t)) - (phpactor :test (lambda () (and (require 'phpactor nil t) (featurep 'phpactor))) + ;; `php-ide-phpactor' is this package's own bridge and defines the two + ;; entry points below; php-ide.el only pulls it in at compile time, so + ;; load it here rather than relying on the package autoloads being present. + (phpactor :test (lambda () (and (require 'phpactor nil t) + (require 'php-ide-phpactor nil t) + (featurep 'phpactor))) :activate php-ide-phpactor-activate :deactivate php-ide-phpactor-deactivate) (eglot :test (lambda () (and (require 'eglot nil t) (featurep 'eglot))) diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 3a34d2ab..0de47cf0 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -1081,6 +1081,19 @@ the feature that was really running." (should (equal '(activated-ok deactivated-ok) (reverse php-mode-test--ide-log))))))) +(ert-deftest php-ide-test-phpactor-feature-loads-its-own-bridge () + "The `phpactor' feature must load php-ide-phpactor.el itself. + +php-ide.el only requires it at compile time, so without this the +`:activate'/`:deactivate' symbols resolve only when the package autoloads +happen to be loaded; loading php-ide.el directly gave `void-function'." + (skip-unless (require 'phpactor nil t)) + (let ((plist (cdr (assq 'phpactor php-ide-feature-alist)))) + (should (funcall (plist-get plist :test))) + (should (featurep 'php-ide-phpactor)) + (should (fboundp 'php-ide-phpactor-activate)) + (should (fboundp 'php-ide-phpactor-deactivate)))) + (ert-deftest php-ide-test-feature-alist-arity () "Regression test: `:test' must always be a callable 0-arg predicate, and for every PHP-IDE feature actually available in this Emacs, `:activate' From 560084fa9062acbab517bba13e5ecf48da84edb0 Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Wed, 5 Aug 2026 20:21:15 +0900 Subject: [PATCH 3/6] Add CHANGELOG entries for the php-ide state-tracking fixes --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba450ed6..dd75a6c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,14 @@ All notable changes of the PHP Mode 1.19.1 release series are documented in this * `php-ide-phpactor-disable-hover-at-point-functions` was combined with AND, so an empty list disabled hover everywhere and multiple predicates only fired when all matched * Fix `php-ide-mode` deactivation stopping Phpactor hover in unrelated buffers * The hover timer is shared by all buffers but was cancelled unconditionally; it is now retired only once no live buffer uses hover + * Fix `php-ide-mode` staying on after a feature failed to activate + * The mode line claimed PHP-IDE was running while nothing had started, and turning it back off then signalled `void-function`. Activation now rolls back and leaves the mode off + * Fix `php-ide-mode` deactivating the wrong feature after `php-ide-features` changes + * Deactivation re-read the variable, so editing `.dir-locals.el` and re-applying it to a live buffer stranded the feature that was really running; it now tracks what it activated + * Fix `php-ide-mode` activating features twice when it is re-enabled + * `hack-local-variables-hook`, where the documented recipe puts `php-ide-turn-on`, runs again on `revert-buffer` and friends + * Fix the `phpactor` PHP-IDE feature failing to activate outside a package installation + * `php-ide.el` requires `php-ide-phpactor` only at compile time, so its `:activate`/`:deactivate` resolved only via the package autoloads; the feature now loads it from `:test` ### Deprecated From 0ab4fd5eef899792fe7683a496ed3265cca00862 Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Wed, 5 Aug 2026 21:43:44 +0900 Subject: [PATCH 4/6] Mark the alists that decide what php-ide runs as risky Restricting the :safe predicates of php-ide-features and php-ide-eglot-executable to "what this package already knows about" only holds while a project cannot extend what the package knows. Neither backing alist was risky, so a .dir-locals.el could add an entry to php-ide-lsp-command-alist and then name it in php-ide-eglot-executable, where our own predicate would wave it through as a bundled preset. php-ide-feature-alist is the same story for the functions php-ide-mode calls. Both are now risky, so Emacs always confirms them and never remembers them as safe. The flag is stated with an explicit autoloaded `put' as well as :risky, because the autoloads generator copies only :safe, and the check that matters runs before php-ide.el is loaded. php-ide-lsp-command-alist becomes a defcustom in passing: adding a preset is how a user makes a server selectable from .dir-locals.el without being asked to confirm an executable path. --- lisp/php-ide.el | 49 ++++++++++++++++++++++++++++++++---------- tests/php-mode-test.el | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/lisp/php-ide.el b/lisp/php-ide.el index 62655296..6fd17e4e 100644 --- a/lisp/php-ide.el +++ b/lisp/php-ide.el @@ -144,29 +144,56 @@ each bound to a function called with no arguments: `:test' Return non-nil when FEATURE is usable in this Emacs, loading its backing package if necessary. `:activate' Turn FEATURE on in the current buffer. -`:deactivate' Turn FEATURE off in the current buffer.") +`:deactivate' Turn FEATURE off in the current buffer. + +`:exclusive' Optional tag. `php-ide-mode' warns when it is asked to + activate more than one feature sharing a tag, since they + would fight over the same buffer.") + +;; The functions in this alist run automatically, and the `:safe' predicate of +;; `php-ide-features' trusts whatever it names, so a directory-local value here +;; would decide both what runs and what counts as safe. Risky variables are +;; always confirmed and can never be remembered as safe. +;;;###autoload +(put 'php-ide-feature-alist 'risky-local-variable t) + +(defgroup php-ide nil + "IDE-like support for PHP developing." + :tag "PHP-IDE" + :prefix "php-ide-" + :group 'php) ;; Autoloaded for the same reason as `php-ide-feature-alist'; the `:safe' -;; predicate of `php-ide-eglot-executable' consults this alist. +;; predicate of `php-ide-eglot-executable' consults this alist. It is risky +;; for the sharper reason that it decides which command gets executed, and +;; adding an entry to it also makes that entry pass as a safe value of +;; `php-ide-eglot-executable' --- so a directory-local value here would hand +;; the directory both halves of that decision. ;;;###autoload -(defvar php-ide-lsp-command-alist +(defcustom php-ide-lsp-command-alist '((intelephense "intelephense" "--stdio") (phpactor . (lambda () (list (if (fboundp 'phpactor--find-executable) (phpactor--find-executable) "phpactor") "language-server")))) - "Alist of bundled LSP server presets for `php-ide-eglot-executable'. + "Alist of LSP server presets available to `php-ide-eglot-executable'. Each element is (NAME . COMMAND), where COMMAND is either a list of strings to execute or a function of no arguments returning such a list. Only the NAME symbols listed here are accepted as safe directory-local -values; see `php-ide-eglot-executable'.") - -(defgroup php-ide nil - "IDE-like support for PHP developing." - :tag "PHP-IDE" - :prefix "php-ide-" - :group 'php) +values; see `php-ide-eglot-executable'. Adding your own preset here is +therefore also how you let a project select it from .dir-locals.el +without being asked to confirm an executable path." + :tag "PHP-IDE LSP Command Alist" + :risky t + :type '(alist :key-type symbol + :value-type (choice (repeat string) function))) + +;; `:risky' above only takes effect once php-ide.el is loaded: unlike `:safe', +;; the autoloads generator does not copy it. Emacs checks .dir-locals.el +;; before that, so state it separately where the check can see it. +;;;###autoload +(put 'php-ide-lsp-command-alist 'risky-local-variable t) ;;;###autoload (defcustom php-ide-features nil diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 0de47cf0..530427a4 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -1172,6 +1172,49 @@ arbitrary function, path, or command list." ;; normal risky-variable confirmation. (should-not (get 'php-ide-mode-functions 'safe-local-variable)))) +(ert-deftest php-ide-test-command-holding-variables-are-risky () + "The alists that decide what PHP-IDE runs must be risky. + +`php-ide-lsp-command-alist' holds the command lines Eglot executes, and +adding an entry to it also makes that entry pass the `:safe' predicate of +`php-ide-eglot-executable' -- so a directory-local value would choose +both the command and its own approval. `php-ide-feature-alist' likewise +holds the functions `php-ide-mode' calls and backs the `:safe' predicate +of `php-ide-features'. Risky variables are always confirmed and can +never be remembered as safe." + (should (risky-local-variable-p 'php-ide-feature-alist)) + (should (risky-local-variable-p 'php-ide-lsp-command-alist)) + ;; And they must not also claim to be safe. + (should-not (get 'php-ide-feature-alist 'safe-local-variable)) + (should-not (get 'php-ide-lsp-command-alist 'safe-local-variable))) + +(ert-deftest php-ide-test-risky-local-variables-work-from-autoloads () + "Regression test: those variables must already be risky before +php-ide.el is loaded. + +Emacs checks .dir-locals.el first, and unlike `:safe' the autoloads +generator does not copy a defcustom's `:risky' flag, so the flag has to +be stated where the check can see it." + (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 + (when (featurep 'php-ide) + (error "php-ide must not be loaded in this check")) + (dolist (v '(php-ide-feature-alist + php-ide-lsp-command-alist)) + (unless (risky-local-variable-p v) + (error "%s is not risky without php-ide loaded" v))) + (princ "OK")))))) + (should (string-match-p "OK" (buffer-string))) + (should (eq 0 status)))))) + (ert-deftest php-ide-test-safe-local-variables-work-from-autoloads () "Regression test: the `:safe' predicates must work from the package autoloads file alone. From 6f1212234442a898e975439fc455e26444e960a0 Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Wed, 5 Aug 2026 21:46:03 +0900 Subject: [PATCH 5/6] Warn about conflicting IDE features and soften Eglot deactivation php-ide-features is a set, so nothing stopped two LSP clients from being enabled together and each trying to manage the same buffer. Tag the clients with :exclusive and warn once when more than one of a tag is requested, while still activating them: an unusual combination stays possible, it just says so. Phpactor's bridge is not an LSP client and pairs with one legitimately, so it is untagged and stays quiet. Eglot's :deactivate also called eglot--managed-mode-off directly. It is internal, and it is still the only way to release one buffer without shutting down a server other buffers use, so php-ide-eglot-deactivate now wraps it: it falls back to eglot--managed-mode and finally to a warning that names eglot-shutdown, rather than failing with void-function if a future Eglot drops it. php-ide-eglot-managed-modes becomes a defcustom while nearby. --- lisp/php-ide.el | 59 +++++++++++++++++++++++++++++++++++------- tests/php-mode-test.el | 47 +++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/lisp/php-ide.el b/lisp/php-ide.el index 6fd17e4e..79257830 100644 --- a/lisp/php-ide.el +++ b/lisp/php-ide.el @@ -124,18 +124,16 @@ :deactivate php-ide-phpactor-deactivate) (eglot :test (lambda () (and (require 'eglot nil t) (featurep 'eglot))) :activate php-ide-eglot-activate - ;; `eglot--managed-mode-off' is Eglot's own internal (and unexported) function, - ;; but it is the only operation that turns Eglot off for just the current buffer - ;; without shutting down a server that other buffers may still be using. The - ;; public `eglot-shutdown' always kills the whole server, which would be a much - ;; more disruptive (and asymmetric) deactivation than `php-ide-eglot-activate'. - :deactivate eglot--managed-mode-off) + :deactivate php-ide-eglot-deactivate + :exclusive lsp-client) (lsp-bridge :test (lambda () (and (require 'lsp-bridge nil t) (featurep 'lsp-bridge))) :activate (lambda () (lsp-bridge-mode +1)) - :deactivate (lambda () (lsp-bridge-mode -1))) + :deactivate (lambda () (lsp-bridge-mode -1)) + :exclusive lsp-client) (lsp-mode :test (lambda () (and (require 'lsp nil t) (featurep 'lsp))) :activate lsp - :deactivate lsp-disconnect)) + :deactivate lsp-disconnect + :exclusive lsp-client)) "Alist of PHP-IDE features and how to probe and (de)activate each one. Each element is (FEATURE . PLIST), where PLIST holds these keywords, @@ -243,11 +241,13 @@ without being asked to confirm an executable path." ((functionp command) (funcall command)) ((listp command) command)))))) -(defvar php-ide-eglot-managed-modes '(php-mode phps-mode php-ts-mode) +(defcustom php-ide-eglot-managed-modes '(php-mode phps-mode php-ts-mode) "Major modes keyed by the `eglot-server-programs' entry php-ide adds. `php-ide-eglot-activate' registers `php-ide-eglot-executable' for exactly -these modes.") +these modes." + :tag "PHP-IDE Eglot Managed Modes" + :type '(repeat symbol)) (defun php-ide-eglot--contact-function (&optional _interactive _project) "CONTACT function registered into `eglot-server-programs' by php-ide. @@ -271,6 +271,22 @@ is unset are unaffected and keep using Eglot's default." eglot-server-programs))) (eglot-ensure)) +(defun php-ide-eglot-deactivate () + "Turn Eglot off in this buffer, leaving other buffers connected. + +Eglot exposes no public way to do exactly that: `eglot-shutdown' kills +the whole server, which other buffers may still be using, and would make +deactivation far more destructive than `php-ide-eglot-activate' was. So +this uses Eglot's internal buffer-scoped switch, and says so rather than +failing with `void-function' if a future Eglot renames it." + (cond + ((fboundp 'eglot--managed-mode-off) (eglot--managed-mode-off)) + ((fboundp 'eglot--managed-mode) (eglot--managed-mode -1)) + (t (lwarn 'php-ide :warning + "Cannot deactivate Eglot in this buffer: this Eglot provides \ +neither `eglot--managed-mode-off' nor `eglot--managed-mode'. Use \ +\\[eglot-shutdown] to stop the server for every buffer it manages.")))) + (defcustom php-ide-mode-lighter " PHP-IDE" "Mode line indicator for `php-ide-mode'. @@ -311,6 +327,28 @@ when .dir-locals.el is edited and re-applied to a live buffer.") (php-ide--activate-features) (php-ide--deactivate-features))) +(defun php-ide--warn-about-exclusive-features (ide-features) + "Warn if IDE-FEATURES asks for features that cannot share a buffer. + +Features tagged with the same `:exclusive' value in +`php-ide-feature-alist' -- the LSP clients, which would each try to +manage the buffer -- are reported once, and activation continues so a +deliberate combination is still possible." + (let ((groups nil)) + (dolist (feature ide-features) + (when-let* ((group (plist-get (cdr (assq feature php-ide-feature-alist)) :exclusive))) + (if-let* ((entry (assq group groups))) + (setcdr entry (cons feature (cdr entry))) + (push (list group feature) groups)))) + (dolist (entry groups) + (when (cdr (cdr entry)) + (lwarn 'php-ide :warning + "`php-ide-features' enables several %s features at once (%s). \ +They will each try to manage this buffer; enabling just one is usually what \ +you want." + (car entry) + (mapconcat #'symbol-name (reverse (cdr entry)) ", ")))))) + (defun php-ide--activate-features () "Activate every feature named by `php-ide-features' in this buffer. @@ -334,6 +372,7 @@ when it is not." unavailable-features (mapconcat (lambda (feature) (concat "'" (symbol-name feature))) (php-ide--available-features) ", "))) + (php-ide--warn-about-exclusive-features ide-features) (condition-case err ;; Every feature in IDE-FEATURES is in `php-ide-feature-alist' here, ;; because the loop above already signalled a `user-error' otherwise. diff --git a/tests/php-mode-test.el b/tests/php-mode-test.el index 530427a4..454ebdf6 100644 --- a/tests/php-mode-test.el +++ b/tests/php-mode-test.el @@ -1172,6 +1172,53 @@ arbitrary function, path, or command list." ;; normal risky-variable confirmation. (should-not (get 'php-ide-mode-functions 'safe-local-variable)))) +(ert-deftest php-ide-test-warns-only-on-exclusive-feature-clashes () + "Enabling two LSP clients at once should warn; other combinations should not. + +Phpactor's bridge is not an LSP client, so pairing it with one is a +legitimate setup and must stay quiet." + (let (warnings) + (cl-letf (((symbol-function 'lwarn) + (lambda (_class _level fmt &rest args) + (push (apply #'format fmt args) warnings)))) + (dolist (features '((eglot) (none) (none phpactor) (eglot phpactor))) + (setq warnings nil) + (php-ide--warn-about-exclusive-features features) + (should-not warnings)) + (dolist (features '((eglot lsp-mode) (lsp-mode lsp-bridge) + (eglot lsp-mode lsp-bridge))) + (setq warnings nil) + (php-ide--warn-about-exclusive-features features) + ;; One warning naming the clashing features, not one per feature. + (should (= 1 (length warnings))) + (dolist (feature features) + (should (string-match-p (regexp-quote (symbol-name feature)) + (car warnings)))))))) + +(ert-deftest php-ide-test-eglot-deactivate-degrades-gracefully () + "`php-ide-eglot-deactivate' must warn, not signal, if Eglot's internal +buffer-scoped switch ever disappears." + (skip-unless (require 'eglot nil t)) + (let (called warned) + (cl-letf (((symbol-function 'eglot--managed-mode-off) + (lambda () (setq called 'managed-mode-off)))) + (php-ide-eglot-deactivate) + (should (eq 'managed-mode-off called))) + ;; Fall back to the minor mode itself when the helper is gone. + (setq called nil) + (cl-letf (((symbol-function 'eglot--managed-mode-off) nil) + ((symbol-function 'eglot--managed-mode) + (lambda (arg) (setq called (cons 'managed-mode arg))))) + (php-ide-eglot-deactivate) + (should (equal '(managed-mode . -1) called))) + ;; With neither available, warn instead of signalling `void-function'. + (cl-letf (((symbol-function 'eglot--managed-mode-off) nil) + ((symbol-function 'eglot--managed-mode) nil) + ((symbol-function 'lwarn) + (lambda (&rest _) (setq warned t)))) + (php-ide-eglot-deactivate) + (should warned)))) + (ert-deftest php-ide-test-command-holding-variables-are-risky () "The alists that decide what PHP-IDE runs must be risky. From 7c9a88cb7f101c06f7ece2ac483db477a54a5918 Mon Sep 17 00:00:00 2001 From: USAMI Kenta Date: Wed, 5 Aug 2026 21:47:05 +0900 Subject: [PATCH 6/6] Say what the php-ide experimental note actually covers "All of these functions, modes and terms are subject to change without notice" told a user configuring php-ide-features nothing useful, and after this round it is no longer true of the options and commands the README documents. Point the caveat at the part that really is in flux: the keywords a php-ide-feature-alist entry may use, and the internals. --- CHANGELOG.md | 8 ++++++++ lisp/php-ide.el | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd75a6c9..833b5b16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,14 @@ All notable changes of the PHP Mode 1.19.1 release series are documented in this * Previously these let a `.dir-locals.el` silently run an attacker-chosen command or Lisp function without Emacs's usual confirmation; now only PHP-IDE's own known feature symbols and bundled executable presets are accepted * **If you now get prompted** by a `.dir-locals.el` you wrote and trust yourself (e.g. `php-ide-eglot-executable` set to a raw path/command, or any use of `php-ide-mode-functions`), this is expected — PHP-IDE can no longer vouch for that value as safe. Answer `!` at the prompt to permanently remember that exact value (all supported Emacs versions), or `+` to trust the whole directory from then on (Emacs 30.1+); see `(info "(emacs) Directory Variables")`. You can also pre-approve values ahead of time in your own init file via `safe-local-variable-values` / `safe-local-variable-directories`, so you are never prompted even on first visit. * `php-ide-mode-functions` was always meant to be configured globally with `add-hook` in your init file (see the Commentary in `php-ide.el`), not set per-project via `.dir-locals.el`. For per-project behavior, branch on the `FEATURE` argument inside your hook function instead of varying the variable's value by directory. + * Mark `php-ide-feature-alist` and `php-ide-lsp-command-alist` as risky local variables + * They decide which command PHP-IDE executes and which functions it calls, and adding an entry to `php-ide-lsp-command-alist` also makes that entry pass the `:safe` check of `php-ide-eglot-executable` — so a project could otherwise have supplied both the command and its own approval. Risky variables are always confirmed and never remembered as safe + * `php-ide-mode` now warns when `php-ide-features` enables more than one LSP client at once + * Activation still proceeds; Phpactor's bridge is not an LSP client and pairs with one without a warning + * Make `php-ide-lsp-command-alist` and `php-ide-eglot-managed-modes` customizable + * Adding your own preset to `php-ide-lsp-command-alist` is also how you let a project select that server from `.dir-locals.el` without confirming an executable path + * Narrow the "experimental" note in `php-ide.el` to what it actually covers + * The options and commands documented in the README are settled; what may still change is how a feature is described to PHP-IDE (the `php-ide-feature-alist` entry keywords and `php-ide--...` internals) ### Fixed diff --git a/lisp/php-ide.el b/lisp/php-ide.el index 79257830..1c185cb2 100644 --- a/lisp/php-ide.el +++ b/lisp/php-ide.el @@ -26,8 +26,18 @@ ;; PHP Mode integrates LSP Mode (lsp-mode), Phpactor (phpactor.el) and IDE-like tools. ;; ;; **Note**: -;; This feature is under development and experimental. -;; All of these functions, modes and terms are subject to change without notice. +;; This feature is still experimental. +;; +;; What you configure and call is settled enough to document: the +;; `php-ide-mode' minor mode and `php-ide-turn-on', the `php-ide-set-feature' +;; and `php-ide-status' commands, and the `php-ide-features', +;; `php-ide-eglot-executable', `php-ide-mode-lighter' and +;; `php-ide-mode-functions' options. The README describes them. +;; +;; What may still change without notice is how a feature is described to +;; PHP-IDE: the keywords understood in `php-ide-feature-alist' entries, and +;; anything named `php-ide--...'. Code that only turns PHP-IDE on and off is +;; unaffected; code that registers its own IDE feature may need updating. ;; ;; ## Motivations ;;