Skip to content

Add support for PHP#40

Merged
kezhenxu94 merged 4 commits intoapache:mainfrom
emschu:feature/php
Jul 13, 2021
Merged

Add support for PHP#40
kezhenxu94 merged 4 commits intoapache:mainfrom
emschu:feature/php

Conversation

@emschu
Copy link
Contributor

@emschu emschu commented Jul 9, 2021

This is a preview to add support for PHP header comments with minimal code changes. I'm not sure if it is a good idea to change the CommentStyle struct this way by adding two new properties ensure_after or ensure_before.

My idea is composing the header license text by ensure_after + header + ensure_before - if the RegEx provided in after yields no results.
Are there other (script) languages which require a similar behaviour?

What do you think about this? Or do you want to discuss this in a dedicated issue?

Some notes:

  • declare(strict_types=[0|1]); has to be omitted or it has to be the very first statement in the script, which is pretty common in modern PHP code.
  • empty php files are considered
  • PHP can be configured to use a short-tag syntax by enabling short_open_tag in php.ini. <?php then can be written as <?, but even with short_open_tags set to On <?php is still a valid start of a PHP code block.

@kezhenxu94 kezhenxu94 added this to the 0.2.0 milestone Jul 9, 2021
@wu-sheng wu-sheng requested review from heyanlong and kezhenxu94 July 9, 2021 12:44
@wu-sheng
Copy link
Member

wu-sheng commented Jul 9, 2021

@heyanlong This is for license check for PHP, welcome to provide feedback.

@kezhenxu94
Copy link
Member

@emschu hey, thanks for bringing this up.

Is it allowed to have multiple <?php ... ?> in a single .php file, in another word, is the following codes are valid in PHP?

<?php
/**
 * header license comments
 */
?>

<?php
// some real codes
?>

If it's valid, maybe we can simple use the following comment style config,

- id: PHP
  start: |
    <?php
    /*
  middle: ' *'
  end: |
    */
    ?>

multi-line strings in yaml is rather legit and normal.

@emschu
Copy link
Contributor Author

emschu commented Jul 9, 2021

@kezhenxu94 Yes, your example is a valid php script syntax. You can read about this topic here.

If a file contains only PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script.

For this reason I think it is not a good idea to generally add a php closing tag after the license header in PHP files as this could cause trouble with whitespace or output buffering in pure php files (pure means: non-template PHP code, e.g. class definitions etc.).

I will think about this problem next week again. Enjoy the weekend!

heyanlong
heyanlong previously approved these changes Jul 12, 2021
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks @emschu !! 🙇🏻

@kezhenxu94 kezhenxu94 changed the title [PREVIEW] Add support for PHP Add support for PHP Jul 13, 2021
@kezhenxu94 kezhenxu94 merged commit 1b91757 into apache:main Jul 13, 2021
@emschu emschu deleted the feature/php branch July 13, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants