search

emacs-php / php-mode

A powerful and flexible Emacs major mode for editing PHP scripts
GNU General Public License v3.0
582 stars 117 forks source link

Feature request : integrate phpactor #451

Open kermorgant opened 6 years ago

kermorgant commented 6 years ago

Hi,

I've very recently discovered the phpactor tool (completion and refactoring tool) : http://phpactor.github.io/phpactor/ (edit)

There are probably several interesting features that could be used, but looking for non overlapping ones, I'd love to have "class move" and "class copy" with automatic update of namespace (and name if copy). But maybe this has more to do with dired that php-mode ?

Sadly, I lack both the elisp knowledge and time to figure that out by myself...

Regards and thanks for all the work here ! Mikael

MarTango commented 6 years ago

Whoa nice find! I'm not sure if this would belong in php-mode itself, but it would be great to have a package for this!

zonuexe commented 6 years ago

@kermorgant Thanks for suggestion. The page looks like 404, is it the same as https://github.com/phpactor/phpactor ?

zonuexe commented 6 years ago

ok, I found the page in http://phpactor.github.io/phpactor/

ejmr commented 6 years ago

@kermorgant

This looks like a great tool. Thank you for bringing it to the attention of people like myself who were completely unaware of it.

Sadly, I lack both the elisp knowledge and time to figure that out by myself...

If you do not have the time, that is completely understandable. However, if you could feasibly set aside any small amount of time, then I personally believe the task of adding support for Phpactor could be an educational way to learn:

I always try to avoid speaking for other people, but I feel very confident in saying that @zonuexe would be happy to have you (or anyone else like @MarTango) contribute work for integrating Phpactor, even if you cannot fully complete the work yourself. Do not hesitate to submit your best effort on that task, even if it's incomplete or buggy. As I have said elsewhere, I am no longer an active contributor to PHP Mode; but lately I've begun to look over the project for about thirty to ninety minutes each weekend. I would be happy to use that time on weekends to review any code you submit for this feature, providing appropriate feedback to help you learn more about Emacs Lisp programming.

@zonuexe

My two cents: I agree with @MarTango that supporting Phpactor should be in a new package, at least temporarily; after it reaches an appropriate level of maturity---as the lead maintainer you are obviously to person to decide whenever Phpactor support reaches that level---then it may be worth including directly into PHP Mode itself. Not necessarily within php-mode.el, but as a separate file in the repository which would come bundled with PHP Mode and which users could optionally enable.

zonuexe commented 6 years ago

As I was working on PHPStan and Flycheck work this week I have not decided on the interface with Phpactor yet, but I am reading Vim script and thinking about it gradually.

ejmr commented 6 years ago

I looked over the Vim script too. Some random thoughts:

kermorgant commented 6 years ago

@ejmr

By a happy coincidence, I decided to take a try last night and wrote my first emacs functions for class:copy and class:move. 

(defun php-class-copy ()
  "Copy a class using phpactor"
  (interactive)
  (setq targetpath (read-file-name "Enter new file name:"))
  (let ((default-directory (projectile-project-root)))
    (start-process "phpactor"
                   (get-buffer-create "*phpactor*")
                   "phpactor"
                   "class:copy"
                   (buffer-file-name (window-buffer (minibuffer-selected-window)))
                   targetpath
                   )
    )
)

(defun php-class-move ()
  "Rename a class using phpactor"
  (interactive)
  (setq targetpath (read-file-name "Enter new file name:"))
  (let ((default-directory (projectile-project-root)))
    (start-process "phpactor"
                   (get-buffer-create "*phpactor*")
                   "phpactor"
                   "class:move"
                   (buffer-file-name (window-buffer (minibuffer-selected-window)))
                   targetpath
                   )
    )
  )

They work when you call them for the currently openened buffer. Worked fine for me, but it assumes you have projectile and phpactor in the PATH.

I'm not so happy with the phpactor buffer collectling logs without showing them

I wonder if there are emacs core functions for copy and move that could be overridden so that dired, ranger or whatever would rely on them for php files. That was the way I imagined it first.

kermorgant commented 6 years ago

@ejmr and thank you for your proposition of reviewing code if I could propose something. I can't promise anything as I could at best gather some limited time during weekends. On the other hand, if there is an "emacs area" that I could see myself working on, this is definitly the one.

MarTango commented 6 years ago

I'm considering attempting to translate the vimscript functions into emacs-lisp, as an exercise for myself to learn emacs-lisp. I don't have any experience writing emacs packages though, so I'm not sure about structure.

(Also, I don't know vimscript, but from what I've seen it looks fairly easy to understand)

ejmr commented 6 years ago

@kermorgant

Thanks for taking the initiave. Sometime during the week, if I can, I will write some notes about the code with some suggestions and friendly criticisms.

@MarTango

This is a great reference for Vimscript in my opinion, which may be helpful for any attempt to port Phpactor's Vim support to Emacs Lisp.

zonuexe commented 6 years ago

I started making phpactor.el. Implementation is still necessary, but at least goto_definition command has worked. Even with this command I am seeing better behavior than past TAGS.

MarTango commented 6 years ago

@zonuexe Amazing! I may attempt to contribute, but my emacs-lisp ability is subpar.

I have a question about some of the code which I don't understand (I tried googling but couldn't find an answer):

;;;###autoload
(progn
  (defvar phpactor-executable nil
    "Path to `phpactor' executable file.")
  (make-variable-buffer-local 'phpactor-working-dir)
  #'(lambda (v) (if (consp v)
                    (and (eq 'root (car v)) (stringp (cdr v)))
                  (null v) (stringp v))))
  1. How does the ;;;###autoload magic comment interact with the progn? From my understanding, setting up something (a function) to be autoloaded prevents the rest of the file from being loaded until the function gets called, but I'm not sure what happens here.
  2. Should (make-variable-buffer-local 'phpactor-working-dir) be (make-variable-buffer-local 'phpactor-executable)?
  3. What is the purpose of the last expression in the BODY of the progn? (#'(lambda (v) .. ))? I'm aware that progn returns the value of the lambda, but what does the lambda actually do? (I'm guessing this is related to the autoload in some way?

Sorry if this was the wrong place to post, I'm not sure if questions like this are supposed to be new issues, or private messages, or something else.

Thanks!

ejmr commented 6 years ago

@MarTango

How does the ;;;###autoload magic comment interact with the progn?

The magic comment creates an appropriate (autoload ...) call within loaddefs.el. But this only happens when the comment preceeds certain specific forms, e.g. defun, defmacro, et cetera. If the magic comment preceeds a form which is not one of those types, then the form (the progn in this case) is copied verbatim into loaddefs.el. The reference manual link below lists all of the forms for which ;;;###autoload creates a corresponding (autoload ...), and anything not in that list is copied verbatim if it follows the magic comment. And simple mnemonic to help remember which is which: only those forms which begin with something named def... or define... result in an (autoload ...) call, e.g. defun, defcustom, cl-defmacro, define-derived-mode, easy-mmode-define-minor-mode, and so on. If the form does not have def or define as part of its name, like progn, then the magic comment does not create an (autoload ...) and simply copies that form verbatim.

Emacs Lisp Reference Manual, Section 15.5: Autoload

ejmr commented 6 years ago

Sorry if this was the wrong place to post, I'm not sure if questions like this are supposed to be new issues, or private messages, or something else.

@zonuexe

I believe questions such as the ones by @MarTango belong in the discussion for the relevant issues or pull-requests, to help educate and improve the skills of people contributing to PHP Mode or who are thinking about contributing. But if you believe it would create too much noise then I would have no issue with you moving these conversations elsewhere. Just my two cents opinion.

zonuexe commented 6 years ago

@MarTango Thanks for your comments.

How does the ;;;###autoload magic comment interact with the progn?

It is as the comment of @ejmr. This variable is accompanied by ###autoload in order to avoid issuing a warning against the directory local variable .dir-locals.el.

Should (make-variable-buffer-local 'phpactor-working-dir) be (make-variable-buffer-local 'phpactor-executable)?

This is just typo. Thanks to that point, I was fixed at https://github.com/emacs-php/phpactor.el/commit/64a12dabc1ef37dad567a739c96ac2911401d7c2 .

What is the purpose of the last expression in the BODY of the progn?

This is wrong code, too. I was fixed at https://github.com/emacs-php/phpactor.el/commit/55dede0281f1729fef044a7bef2e8a6b8eb72b7f .

This is also a safe-local-variable checker, but I was in a hurry yesterday night, so I did not notice that this was not an accurate formula. The original code does not affect anything, it is not a special expression for autoload.

zonuexe commented 6 years ago

@ejmr https://github.com/ejmr/php-mode/issues/451#issuecomment-381778807 I think so, too.

@MarTango We want to increase collaborators and reviewers, so I would like you to please ask us all your questions. I never feel your comment as noise and I hope that you will continue to cooperate. 👍

MarTango commented 6 years ago

What are the advantages/disadvantages of phpactor over php-language-server? I'm under the impression that php-language-server has a higher memory footprint, but I'm not sure if that's true.

dantleech commented 6 years ago

:wave: hi, I'm the maintainer of Phpactor

What are the advantages/disadvantages of phpactor

PHP language server, is a server - last time I checked it sits there and builds up an in-memory index of all your PHP files in real-time (using co-routines so it can still serve requests).

Phpactor is not a server, it spins up for each request.

Language server should have a larger memory footprint and a warm-up period but will be a bit more comprehensive when locating classes and function.

When used with composer, Phpactor will be blind to multiple class declarations in the same file. For a simple completion request it has a memory usage of around 11MB. Currently Phpactor is only aware of the in-built PHP functions, but hopefully support for user functions will come soon.

PHP Language Server implements the Language Server protocol, Phpactor implements it's own RPC protocol (LSP support might be added in the future).

Phpactor also has many other features outside of completion and jumping to definitions, which you could use alongside the PHP Language Server if you wanted.

Other than that, I havn't really used the PHP Language Server, so can't comment more on it, but looks like a great project.

ejmr commented 6 years ago

@dantleech

Thank you, first for creating Phpactor, and second for the information you've provided. It is my understanding that you are the principle (sole?) author of Phpactor's Vim plugin; is that correct? If so, could you please describe any challenges involved in creating the plugin, along with any limitations you may have found? I believe any such information would be greatly useful to the developers here who want to make Phpactor easy to use for programmers using GNU Emacs. Thank you in advanced for any insight you can share.

dantleech commented 6 years ago

The plugin itself is pretty straight-forward, most of the complication is in dispatching the RPC response to actions (e.g. replace_file_source, file_references) and then the creation of inputs (e.g. choice and text).

Some things that might be relevant:

  1. Menus: VIM has a choice menu which gives you a numeric option, but in some situations this doens't make sense, e.g. in the context menu, where it makes more sense to have letters:
Class "FunctionFormatter":
[o]verride_method, (r)eplace_references, (f)ind_references, (m)ove, (c)opy, (i)nflect, c(l)ass_new, (t)ransform_file, (g)oto_definition, (n)avigate, im(p)ort:

This functionality is provided by VIMs confirm function, but it requires that you manually specify which character should be the shortcut. The Phpator VIM plugin does this automatically. This works pretty well, but if we introduce new options then the shortcuts are liable to change.

  1. Symbol References

VIM has the concept of a "quick fix" list which can be populated with file paths and locations, when you search for references in Phpactor (through the plugins file_references action), it populates the quick fix list with the references. I guess there is some equivilent functionality in Emacs?

  1. Actions on selections

Phpactor has an "extract method" feature, which operates on a selection of code, but currently the context menu doesn't handle selections, so it must be mapped to a shortcut and executed explicitly. This is really a problem that could be solved in Phpactor, but it's different from the normal context menu which operates an an offset rather than a selection.

Those are the things I can think of now, feel free to ask any questions :)