Fix and document php-ide, and complete its Eglot integration - #826
Merged
Conversation
The custom-set-variables example used invalid quoting for `php-ide-features' and the per-project snippet, and the "no IDE support" note referenced a variable name that never existed (`php-ide-feature`).
- phpactor's :deactivate pointed at php-ide-phpactor-activate instead of php-ide-phpactor-deactivate, so turning php-ide-mode off actually re-activated Phpactor - lsp-mode's :deactivate called lsp-workspace-shutdown, which requires a WORKSPACE argument and errored on every deactivation; switched to the buffer-scoped lsp-disconnect - php-ide-eglot-server-program used the assq cons cell itself instead of its cdr, so any symbolic php-ide-eglot-executable value (e.g. intelephense, phpactor) produced a malformed command list - Add php-ide-eglot-activate, which buffer-locally registers php-ide-eglot-executable into eglot-server-programs; previously the variable had no effect on Eglot at all
- Restrict :safe on php-ide-features/php-ide-eglot-executable to PHP-IDE's own known feature symbols and bundled executable presets, and drop :safe from php-ide-mode-functions entirely; previously these let a .dir-locals.el silently run an attacker-chosen command or Lisp function without Emacs's usual confirmation - php-ide-mode now accepts php-ide-features set to a bare symbol, not just a list, matching what the Commentary already showed - php-ide-turn-on no longer signals a user-error when php-ide-features is unset; it is now a silent no-op, so it is safe to add unconditionally to hack-local-variables-hook - Remove now-unreachable dead code in php-ide-mode's activation loop and rename php-ide--avilable-features (typo) to php-ide--available-features - Add php-ide-set-feature and php-ide-status commands to interactively choose/enable an available feature and report the current state
Add a PHP-IDE section covering configuration, the interactive commands (php-ide-mode, php-ide-turn-on, php-ide-set-feature, php-ide-status), per-project setup, and which values are safe in .dir-locals.el.
Cover php-ide-eglot-server-program's resolution of every php-ide-eglot-executable shape, the feature-alist's :test/:activate/ :deactivate arity contract, the phpactor activate/deactivate fix, bare -symbol php-ide-features, php-ide-turn-on's no-op behavior, the new php-ide-set-feature/php-ide-status commands, the :safe predicates, and php-ide-eglot-activate's buffer-local eglot-server-programs registration.
Emacs decides whether a .dir-locals.el value is safe while hacking local
variables, which happens *before* php-ide.el gets loaded: the documented
recipe only pulls it in from hack-local-variables-hook, which runs
afterwards. The predicates therefore execute as copied into
php-mode-autoloads.el, where neither cl-lib nor php-ide.el's own
variables are available yet:
php-ide-features -> (void-function cl-loop)
php-ide-eglot-executable -> (void-variable php-ide-lsp-command-alist)
safe-local-variable-p demotes those errors and returns nil, so both
variables were treated as unsafe and prompted for confirmation in every
project that set them -- exactly the per-project workflow the README
documents.
Autoload the two alists so the predicates can consult them from the
autoloads file, and rewrite the predicates without cl-lib. Also give
both alists the docstrings they should have had as public variables.
php-ide-phpactor--disable-hover-at-point-p promises to return non-nil when *any* predicate in php-ide-phpactor-disable-hover-at-point-functions matches, but was written as `never (not ...)', i.e. logical AND. An empty list therefore suppressed hover everywhere -- the opposite of the intent -- and several predicates only fired when all of them matched. The stock one-element value happens to behave the same either way, so this only bit users who customized the variable. Switch to `thereis' and document the variable.
The hover timer is a single global object shared by every buffer, but php-ide-phpactor-deactivate cancelled it unconditionally. Turning php-ide-mode off in one PHP buffer therefore silently stopped hover in every other one, while those buffers still believed it was active. Cancel it only once no live buffer has hover enabled, and let the timer function retire the timer itself, so a buffer killed while active does not leave it running forever. Also clear the buffer-local hover cache on deactivation.
php-ide-mode-lighter described itself as "A symbol of PHP-IDE feature", which says nothing about the mode line string it actually holds. The remaining undocumented defvars now have docstrings, and php-ide-eglot-managed-modes opens with a complete sentence so checkdoc is clean.
Emacs 28 was the first to copy a defcustom's :safe predicate into the generated autoloads file, so on Emacs 27 -- still supported -- the subprocess found no predicate to call and the test failed rather than skipping. Mirrors the guard the equivalent php-complete test uses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
php-idehas been experimental since 1.24.3 and had seen little use, so several of its advertised features never actually worked. This fixes them, completes the Eglot integration, and documents the feature.Fixed
phpactornever deactivated. Its:deactivatepointed atphp-ide-phpactor-activate, so turningphp-ide-modeoff re-activated Phpactor.lsp-modedeactivation always errored.lsp-workspace-shutdownrequires aWORKSPACEargument; switched to the buffer-scopedlsp-disconnect.php-ide-eglot-executablevalues were unusable. Theassqresult was used withoutcdr, sointelephense/phpactorproduced a malformed command list.php-ide-eglot-executablehad no effect at all. Nothing ever registered it intoeglot-server-programs;php-ide-eglot-activatenow does, buffer-locally.:safepredicates never applied. Emacs checks directory-local values beforephp-ide.elloads, so they ran as copied into the autoloads file and hitvoid-function cl-loop/void-variable.safe-local-variable-pdemotes such errors to nil, so every project setting these variables was prompted for confirmation anyway.php-ide-phpactor-disable-hover-at-point-functionswas combined with AND despite documenting "any", so an empty list disabled hover everywhere.Security
php-ide-mode-functionsholds functions thatphp-ide-modecalls automatically, and its:safepredicate accepted anyfboundpsymbol — enough for a repository to run code in a visitor's Emacs just by having them open a file. It now has no:safepredicate.php-ide-featuresandphp-ide-eglot-executableare restricted to this package's own known feature symbols and bundled presets, so a raw executable path in.dir-locals.elno longer applies silently.The variable was always meant to be set globally with
add-hook, as its own Commentary shows, so this costs no documented use case. Users who do want a directory-local value can approve it through Emacs' normal mechanisms (!at the prompt, orsafe-local-variable-directorieson Emacs 30.1+); the CHANGELOG spells this out.Changed
php-ide-turn-onis a no-op whenphp-ide-featuresis unset, instead of auser-error. The README recipe adds it tohack-local-variables-hook, so it previously errored on every PHP file until a feature was configured.php-ide-featuresnow also accepts a bare symbol, as its own Commentary already showed.Added
php-ide-set-feature— pick a feature interactively, among those actually available.php-ide-status— report whether PHP-IDE is on, what is configured, and what is available.php-idehad no documentation beyond one line in a config sample.Testing
eask test ert ./tests/php-mode-test.elpasses (82 tests, 2 skipped). Every fix above has a regression test, including one that runs the:safepredicates in a subprocess loading only the autoloads file.php-ide.elandphp-ide-phpactor.elare clean under byte-compile and checkdoc.