Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add php-mode-maybe for auto-mode-alist #362

Closed
wants to merge 2 commits into from
Closed

Add php-mode-maybe for auto-mode-alist #362

wants to merge 2 commits into from

Conversation

zonuexe
Copy link
Member

@zonuexe zonuexe commented Jun 29, 2017

This change is somewhat strange, but the code is simple.
I kept this patch for four months, but I think that this PR may not be accepted.

The new function feature/integrates-other-mode abandons the work of php-mode under certain conditions and invoke another major mode. This implementation is a bit odd but you can delegate files that are not good for php-mode to other major modes.

One of them is the Blade template. This file generally has .blade.php extension, but it can not be edited in php-mode. Fortunately web-mode supports Blade templates. When web-mode is not installed in Emacs, simply announce warn and notify the user that php-mode is not suitable for editing the blade file.


Back ground 1

Currently, many php-mode users can not open config.php. When php-mode is auto loaded, its priority is lower in auto-mode-alist due to the append flag of add-to-list. Because of that, /config.php matches conf-maybe-mode.

Although I think that the append flag should be removed, I am worried that it may break the end users' setting compatibility.

Back ground 2

Blade template file name has .blade.php extension.
Because the template language has its own control syntax, php-mode is useless.

@zonuexe
Copy link
Member Author

zonuexe commented Jun 29, 2017

This change can be simply replaced by (add-to-list 'auto-mode-alist '("\\.blade\\.php\\'" . php-mode)).

I was wondering if I should abandon this patch. However, it may be able to help some users not familiar with PHP.

@ejmr
Copy link
Collaborator

ejmr commented Jul 1, 2017

I like the concept and I believe this is a useful change we should make. However, my opinion is that it would better if we simply added:

;;;###autoload
(progn
    (add-to-list 'auto-mode-alist '("\\.blade\\.php\\'" . web-mode)
    (add-to-list 'auto-mode-alist '("config\\.php\\'" . php-mode)

We could put this at the end of php-mode.el after our (dolist ...) that affects auto-mode-alist.

Do Blade templates require the extension .blade.php or is that only the common convention? Regardless I think we should make this change. But I have not used Laravel's Blade templates so I am curious of that extension is mandatory.

Right now I think we should take the approach of using (add-to-list 'auto-mode-alist ...) to handle these problems. However, I will personally keep your patch in my local repository in case we decide to use it in the future, or maybe some modified version of it to help notify users when it may be better, for example, to use Web Mode over PHP Mode, or any template-specific major modes, things like that.

@syohex I would really appreciate hearing your opinion on this before making any change. I believe we should introduce additional calls to modify auto-mode-alist for both Blade templates and specifically config.php. But I would like a second opinion before doing this.

@zonuexe Thank you for the patch and raising the issue. For now let's add additional entries to auto-mode-alist for those files, but like I said, I will keep a copy of your patch for the future just in case. If no one has any objections then I will add the auto-mode-alist changes this weekend.

@ejmr
Copy link
Collaborator

ejmr commented Jul 1, 2017

I should also add, I do not believe that modifying auto-mode-alist to call major modes that are not ours is the best idea in terms of style; the user may not have Web Mode at all. So for that reason, instead of using (add-to-list 'auto-mode-alist '("\\.blade\\.php\\'" . web-mode) maybe we should display a big warning to the user in that scenario and describe to them how to get Web Mode for use with Blade templates.

And finally, no matter what we do, let's update the README to describe PHP Mode's lack of usefulness for Blade templates.

@zonuexe
Copy link
Member Author

zonuexe commented Jul 2, 2017

@ejmr
Thank you, I'm glad you could hear your opinion!
I have submitted a change for /config.php in #363.

@zonuexe zonuexe changed the title Add php--integrate-other-mode Add php-mode-maybe for auto-mode-alist Jul 2, 2017
zonuexe and others added 2 commits July 2, 2017 21:47
The new function `php-mode-maybe` is wrapper for auto-mode-alist that
invokes another function that is not php-mode under certain conditions.

One of them is the Bladetemplate.  This file generally has .blade.php
extension, but it can not be edited in php-mode.  Fortunately web-mode
supports Blade templates.  When web-mode is not installed in Emacs,
simply announce warn and notify the user that php-mode is not suitable
for editing the blade file.
Use 'php-mode-maybe instead of 'php-mode.
Invoke php-mode or the appropriate major mode when open the file.
@zonuexe
Copy link
Member Author

zonuexe commented Jul 2, 2017

@ejmr
Sorry, I was forgotten to take the same approach as conf-mode-maybe.
With this changes, without strange changes to major mode, it only affects with dispatch in auto-mode-alist.

@ejmr
Copy link
Collaborator

ejmr commented Jul 2, 2017

@zonuexe Thank you for the pull-requests. I just finished a comment on the other, but I will look over both of them a second time tonight. I will probably make a few small tweaks but also will merge the changes in the next twenty-four hours unless some major problem arises (which I really doubt will happen).

@ejmr
Copy link
Collaborator

ejmr commented Jul 22, 2017

@zonuexe I am very sorry for the long delay in cleaning this up and merging it. I have been having serious health problems---kidney failure---so I simply haven't had the time or energy to write all this month. Nonethelss I still intended to merge this as soon as I feel better and get things under control. I am really sorry about the delay, but thank you very much for being patient.

@zonuexe
Copy link
Member Author

zonuexe commented Jul 23, 2017

@ejmr
No problem. You should care about your own health. I also wish for your health. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants