GNU bug report logs - #78994
31.0.50; [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of JS,CSS and HTML.

Previous Next

To reply to this bug, email your comments to 78994 AT debbugs.gnu.org.
There is no need to reopen the bug first.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of
 JS,CSS  and HTML.
Date: Fri, 11 Jul 2025 17:35:38 +0200

[Message part 1 (text/plain, inline)]

Ciao,
this patch replaces the dependency on {js,css,html}-ts-mode with the 
dependency on ‘mhtml-ts-mode’. 
Code maintenance should now be easier. 
Additional benefits are:
1. Imenu now exposes symbols for all of all languages,
2. navigation now works correctly for all languages,
3. outline works for all languages.
Additional new features are:
1. indentation of PHP in mixed buffers with HTML now works much better and 
allows three different behaviors, an option allows you to choose the behavior.
2. a new feature shows where PHP ini files are both locally and remotely, if 
the buffer is associated with a remote PHP file.

The patch is a little long, but I hope it's okay.
Thank you.

Vincenzo

[0001-php-ts-mode-depends-on-mhtml-ts-mode-instead-of-JS-C.patch (text/x-patch, attachment)]

Message #8 received at 78994 <at> debbugs.gnu.org (full text, mbox):

From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org, 78994 <at> debbugs.gnu.org, eliz <at> gnu.org
Subject: Re: bug#78994: 31.0.50; [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of JS,CSS and HTML.
Date: Tue, 05 Aug 2025 12:59:45 +0200

[Message part 1 (text/plain, inline)]

Eli, any comments on this patch?

Thanks.
Vincenzo 

Il 11 luglio 2025 17:35:38 CEST, Vincenzo Pupillo <v.pupillo <at> gmail.com> ha scritto:
>Ciao,
>this patch replaces the dependency on {js,css,html}-ts-mode with the 
>dependency on ‘mhtml-ts-mode’. 
>Code maintenance should now be easier. 
>Additional benefits are:
>1. Imenu now exposes symbols for all of all languages,
>2. navigation now works correctly for all languages,
>3. outline works for all languages.
>Additional new features are:
>1. indentation of PHP in mixed buffers with HTML now works much better and 
>allows three different behaviors, an option allows you to choose the behavior.
>2. a new feature shows where PHP ini files are both locally and remotely, if 
>the buffer is associated with a remote PHP file.
>
>The patch is a little long, but I hope it's okay.
>Thank you.
>
>Vincenzo

[Message part 2 (text/html, inline)]

Message #14 received at 78994 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vincenzo Pupillo <v.pupillo <at> gmail.com>
Cc: 78994 <at> debbugs.gnu.org
Subject: Re: bug#78994: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead
 of JS,CSS and HTML.
Date: Tue, 05 Aug 2025 16:47:55 +0300

> Date: Tue, 05 Aug 2025 12:59:45 +0200
> From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> 
> Eli, any comments on this patch?

I hoped some user of this mode will be able to review, but I guess
that's not going to happen...

The patch is massive, and the changes quite radical.  But OTOH you are
the author and the maintainer of the mode, so who am I to question
your decisions, especially as I don't program in PHP?

So just a few superficial comments below, mainly about documentation:

> The direct dependence on js-ts-mode, css-ts-mode and html-ts-mode has
> now been replaced by ‘mhtml-ts-mode’.
> Additional benefits are:
> 1. Imenu now exposes symbols for all of all languages,
> 2. navigation now works correctly for all languages,
> 3. outline works for all languages.
> 
> Additional new features are:
> 1. indentation of PHP in mixed buffers with HTML now works much better
> and allows three different behaviors, an option allows you to choose the
> behavior.
> 2. a new feature shows where PHP ini files are both locally and
> remotely, if the buffer is associated with a remote PHP file.

I think at least part of the above should be in NEWS, to explain the
rationale for these changes.

> +*** The option 'php-ts-mode-css-fontify-colors' has been removed.
> +'mhtml-ts-mode-css-fontify-colors' replace this option.
                                      ^^^^^^^
"replaces"

> +*** New user option 'php-ts-mode-find-sibling-rules'.
> +Rules for finding siblings of a PHP files.
                                 ^^^^^^^^^^^
"a PHP file", singular, I believe.

> +*** New user option 'php-ts-mode-phpdoc-highlight-errors'.
> +When not null, it highlights unknown PHPDOC tags using
        ^^^^^^^^
Did you mean "non-nil"?

> +‘font-lock-warning-face’ so that the user can identify them more easily.
   ^^^^^^^^^^^^^^^^^^^^^^^^
Please quote in NEWS 'like this', using ASCII characters.

> +*** New command 'php-ts-mode-show-ini' to show where are located the ini files.

I would delete the part beginning with "to show", since you explain
that in the very next sentence:

> +Show the location of the PHP ini files, if the buffer is associated to a remote
> +PHP file show the remote PHP ini files.


> +(defcustom php-ts-mode-html-indent-offset 2
> +  "PHP indent spaces related to the HTML tags.

I think you mean

  The number of spaces to indent PHP code relative to HTML tags.

> +By default should have same value as `mhtml-ts-mode-js-css-indent-offset'."

Only "by default"?  I'm not sure I understand what you mean by "by
default should" -- do you mean it is not recommended to make it
different from mhtml-ts-mode-js-css-indent-offset?

> +(defcustom php-ts-mode-html-relative-indent t
> +  "How PHP code are indented relative to the HTML tags.
              ^^^^^^^^
Either "codes are" or "code is".

> +  :type '(choice (const nil) (const t) (const ignore))

Please consider adding :tag's to the 3 supported values, because the
values themselves don't really explain their effect.

> +(defcustom php-ts-mode-php-default-executable (or (executable-find "php") "php")
>    "The default PHP executable."

Suggest to clarify:

  "The default file name of the PHP executable."

> +(defcustom php-ts-mode-find-sibling-rules
> +  (list (list (rx "src/" (group (+ not-newline) "/") (group (+ (not "/"))) ".php") "tests/\\1\\2Test.php")
> +	(list (rx "tests/" (group (+ not-newline) "/") (group (+ (not "/"))) "Test.php") "src/\\1\\2.php"))
> +  "Rules for finding sibling files as defined by `find-sibling-rules'.
> +As a default, the rules try to find the corresponding test of the
> +current source file and vice versa.  Source files are assumed to be in
> +src/, and tests of in tests/.  Many frameworks have a folder
> +arrangement similar to this."

Please document in the doc string the form of the value.  Something
like

  Value should be a list whose elements are of the form

      (FILENAME-REGEXP SIBLING-FILE)

  where FILENAME-REGEXP is the regexp matching...

> +(defcustom php-ts-mode-phpdoc-highlight-errors nil
> +  "Highlights tags unknown to the phpdoc parser.
      ^^^^^^^^^^
"Highlight", without the "s".

> +(defun php-ts-mode-show-ini ()
> +  "Show PHP configuration file names."

Suggest to rephrase:

  Pop a buffer showing the PHP configuration file names.

> +Returns 0 if the NODE is after the </html>.  If
   ^^^^^^^
"Return"

> +`php-ts-mode-html-relative-indent' is not nil returns the indentation
                                                 ^^^^^^^
"return"

> +point of the last word before the NODE, plus the indentation offset,
> +otherwise return only the indentation point.  If there is no HTML tag,
> +it returns the beginning of the parent.  When NODE is nil it behave
                                                                ^^^^^^
"behaves"

Message #17 received at 78994 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vincenzo Pupillo <v.pupillo <at> gmail.com>
Cc: 78994 <at> debbugs.gnu.org
Subject: Re: bug#78994: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of
 JS,CSS and HTML.
Date: Thu, 07 Aug 2025 08:29:06 +0300

[You forgot to use Reply All, I think.]

> From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> Date: Wed, 06 Aug 2025 21:52:27 +0200
> 
> > I think at least part of the above should be in NEWS, to explain the
> > rationale for these changes.
> 
> Something like this?
> ---
> *** 'php-ts-mode' now depends on 'mhtml-ts-mode'.
> The direct dependency on js-ts-mode, css-ts-mode and html-ts-mode has
> now been replaced by ‘mhtml-ts-mode’. Navigation, Outline and Imenu work
> for all languages and code maintenance is easier.

Yes.  But remember to use two spaces between sentences.

> > > +By default should have same value as
> > > `mhtml-ts-mode-js-css-indent-offset'."
> > Only "by default"?  I'm not sure I understand what you mean by "by
> > default should" -- do you mean it is not recommended to make it
> > different from mhtml-ts-mode-js-css-indent-offset?
> 
> I mean by default we assign 2 which is the same value as 'mhtml-ts-mode-js-
> css-indent-offset’ so it looks like 'mhtml-ts-mode'.

Then maybe something like

  It is advisable to have this have the same value as
  `mhtml-ts-mode-js-css-indent-offset'.

> > > +(defcustom php-ts-mode-find-sibling-rules
> > > +  (list (list (rx "src/" (group (+ not-newline) "/") (group (+ (not
> > > "/"))) ".php") "tests/\\1\\2Test.php") +	(list (rx "tests/" (group (+
> > > not-newline) "/") (group (+ (not "/"))) "Test.php") "src/\\1\\2.php")) + 
> > > "Rules for finding sibling files as defined by `find-sibling-rules'. +As
> > > a default, the rules try to find the corresponding test of the +current
> > > source file and vice versa.  Source files are assumed to be in +src/, and
> > > tests of in tests/.  Many frameworks have a folder
> > > +arrangement similar to this."
> > 
> > Please document in the doc string the form of the value.  Something
> > like
> > 
> >   Value should be a list whose elements are of the form
> > 
> >       (FILENAME-REGEXP SIBLING-FILE)
> > 
> >   where FILENAME-REGEXP is the regexp matching...
> > 
> In the documentation I wrote that the rules are the same as those defined by 
> 'find-sibling-rules’. It might be redundant to provide an additional 
> explanation.

My impression from reading the doc string is that the rules here are
somewhat different, but if not, then maybe it's enough to make a
simple rewording will make the reference more explicit, like this:

  Rules for finding sibling files.  See `find-sibling-rules' for the
  form of the value.

Message #20 received at 78994 <at> debbugs.gnu.org (full text, mbox):

From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78994 <at> debbugs.gnu.org
Subject: Re: bug#78994: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of
 JS,CSS and HTML.
Date: Thu, 07 Aug 2025 22:07:45 +0200

[Message part 1 (text/plain, inline)]

Ciao Eli, thank you. 
Attached is the new version of the patch. 

Thanks.
Vincenzo

In data giovedì 7 agosto 2025 07:29:06 Ora legale dell’Europa centrale, Eli 
Zaretskii ha scritto:
> [You forgot to use Reply All, I think.]
> 
> > From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> > Date: Wed, 06 Aug 2025 21:52:27 +0200
> > 
> > > I think at least part of the above should be in NEWS, to explain the
> > > rationale for these changes.
> > 
> > Something like this?
> > ---
> > *** 'php-ts-mode' now depends on 'mhtml-ts-mode'.
> > The direct dependency on js-ts-mode, css-ts-mode and html-ts-mode has
> > now been replaced by ‘mhtml-ts-mode’. Navigation, Outline and Imenu work
> > for all languages and code maintenance is easier.
> 
> Yes.  But remember to use two spaces between sentences.
> 
> > > > +By default should have same value as
> > > > `mhtml-ts-mode-js-css-indent-offset'."
> > > 
> > > Only "by default"?  I'm not sure I understand what you mean by "by
> > > default should" -- do you mean it is not recommended to make it
> > > different from mhtml-ts-mode-js-css-indent-offset?
> > 
> > I mean by default we assign 2 which is the same value as
> > 'mhtml-ts-mode-js-
> > css-indent-offset’ so it looks like 'mhtml-ts-mode'.
> 
> Then maybe something like
> 
>   It is advisable to have this have the same value as
>   `mhtml-ts-mode-js-css-indent-offset'.
> 
> > > > +(defcustom php-ts-mode-find-sibling-rules
> > > > +  (list (list (rx "src/" (group (+ not-newline) "/") (group (+ (not
> > > > "/"))) ".php") "tests/\\1\\2Test.php") +	(list (rx "tests/" (group (+
> > > > not-newline) "/") (group (+ (not "/"))) "Test.php") "src/\\1\\2.php"))
> > > > +
> > > > "Rules for finding sibling files as defined by `find-sibling-rules'.
> > > > +As
> > > > a default, the rules try to find the corresponding test of the
> > > > +current
> > > > source file and vice versa.  Source files are assumed to be in +src/,
> > > > and
> > > > tests of in tests/.  Many frameworks have a folder
> > > > +arrangement similar to this."
> > > 
> > > Please document in the doc string the form of the value.  Something
> > > like
> > > 
> > >   Value should be a list whose elements are of the form
> > >   
> > >       (FILENAME-REGEXP SIBLING-FILE)
> > >   
> > >   where FILENAME-REGEXP is the regexp matching...
> > 
> > In the documentation I wrote that the rules are the same as those defined
> > by 'find-sibling-rules’. It might be redundant to provide an additional
> > explanation.
> 
> My impression from reading the doc string is that the rules here are
> somewhat different, but if not, then maybe it's enough to make a
> simple rewording will make the reference more explicit, like this:
> 
>   Rules for finding sibling files.  See `find-sibling-rules' for the
>   form of the value.

[0001-php-ts-mode-depends-on-mhtml-ts-mode-instead-of-JS-C.patch (text/x-patch, attachment)]

Message #25 received at 78994-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vincenzo Pupillo <v.pupillo <at> gmail.com>
Cc: 78994-done <at> debbugs.gnu.org
Subject: Re: bug#78994: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead of
 JS,CSS and HTML.
Date: Sat, 09 Aug 2025 15:58:39 +0300

> From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> Cc: 78994 <at> debbugs.gnu.org
> Date: Thu, 07 Aug 2025 22:07:45 +0200
> 
> Ciao Eli, thank you. 
> Attached is the new version of the patch. 

Thanks, now installed on the master branch, and closing the bug.

Message #28 received at 78994 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: v.pupillo <at> gmail.com
Cc: 78994 <at> debbugs.gnu.org
Subject: Re: bug#78994: 31.0.50;
 [PATCH] php-ts-mode: `php-ts-mode' depends on `mhtml-ts-mode' instead
 of JS,CSS and HTML.
Date: Sat, 09 Aug 2025 16:31:40 +0300

> Resent-To: bug-gnu-emacs <at> gnu.org
> Cc: 78994-done <at> debbugs.gnu.org
> Date: Sat, 09 Aug 2025 15:58:39 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> > Cc: 78994 <at> debbugs.gnu.org
> > Date: Thu, 07 Aug 2025 22:07:45 +0200
> > 
> > Ciao Eli, thank you. 
> > Attached is the new version of the patch. 
> 
> Thanks, now installed on the master branch, and closing the bug.

Unfortunately, I was forced to revert the changeset, since it failed
the build without the tree-sitter library:

  In toplevel form:
  progmodes/php-ts-mode.el:72:11: Error: Symbol’s function definition is void: treesit-query-compile

Line 72 is where it requires mhtml-ts-mode, but I have no idea why
this causes the error.  Could you please look into fixing this?

Message #31 received at 78994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78994 <at> debbugs.gnu.org, v.pupillo <at> gmail.com
Subject: Re: bug#78994: 31.0.50; [PATCH] php-ts-mode: `php-ts-mode' depends
 on `mhtml-ts-mode' instead of JS,CSS and HTML.
Date: Sun, 10 Aug 2025 09:44:54 +0300

> Unfortunately, I was forced to revert the changeset, since it failed
> the build without the tree-sitter library:
>
>   In toplevel form:
>   progmodes/php-ts-mode.el:72:11: Error: Symbol’s function definition is void: treesit-query-compile
>
> Line 72 is where it requires mhtml-ts-mode, but I have no idea why
> this causes the error.  Could you please look into fixing this?

It seems not only `treesit-query-compile` needs a guard
`(when (treesit-available-p) ...)` in the variables default values,
but also `treesit-range-rules` requires such a guard as well.
Can we find a way to avoid such error-prone guards?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.