Take php-executable from php-core instead of guessing at it#818
Merged
Conversation
php-project.el and php-flymake.el both wanted php-executable and
neither could say so. php.el requires php-project, so php-project
requiring php.el back would have been circular, and php-flymake.el is
pulled in by php-mode.el for the same reason. Both worked around it by
testing whether the variable happened to be bound:
php-project.el: ((boundp 'php-executable) php-executable)
(t (executable-find "php"))
php-flymake.el: (list (or (bound-and-true-p php-executable) "php"))
php-core.el now holds php-executable and depends on nothing, so both can
require it and ask for the variable directly. No cycle: php-core
requires nothing, and php.el already requires both php-core and
php-project.
This drops the fallback branches, which were only ever reached when
php.el had not been loaded. The single behaviour change is there: with
no PHP installed, php-project-get-php-executable returned nil in that
case and now returns "php". That is what php-executable itself defaults
to -- (or (executable-find "php") "php") -- so the two now agree instead
of disagreeing.
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.
Follow-up from #815, now that php-core.el exists.
The problem
php-project.el and php-flymake.el both wanted
php-executableand neither could say so. php.el requires php-project, so php-project requiring php.el back would have been circular; php-flymake.el is pulled in by php-mode.el for the same reason. Both worked around it by testing whether the variable happened to be bound:The fix
php-core.el holds
php-executableand depends on nothing, so both files can require it and ask for the variable directly. No cycle — php-core requires nothing, and php.el already requires both php-core and php-project. Verified that(require 'php-project)and(require 'php-flymake)each load standalone.The fallback branches go away. They were only ever reached when php.el had not been loaded, which can no longer happen for this variable.
The one behaviour change
With no PHP installed,
php-project-get-php-executablereturnednilin that php.el-not-loaded case and now returns"php". That is exactly whatphp-executableitself defaults to —(or (executable-find "php") "php")— so the two now agree instead of disagreeing. It has no in-tree callers; it is public API documented in php-project.el's commentary.Verification
/opt/homebrew/bin/php(unchanged)php-executable(unchanged)"php"(wasnilonly when php.el was unloaded)Byte-compiles with no new warnings (the two
rxanywarnings in php-flymake.el are pre-existing and only shift by a line); full suite green.