October: Unterminated comment when running migrations - php artisan october:up

[getrightnl]

Expected behavior

I used the builder plugin to make my migrations and added phpDoc block comments manually. I would expect it to run even with the comments.

Actual behavior

With the comments it is giving errors when running php artisan october:up in the CLI. Without them it is working like it should. See warning below. Related to this closed issue https://github.com/octobercms/october/issues/2421

PHP Warning: Unterminated comment starting line 25 in /var/www/gi/vendor/october/rain/src/Database/Updater.php on line 110 PHP Stack trace: PHP 1. {main}() /var/www/gi/artisan:0 PHP 2. Illuminate\Foundation\Console\Kernel->handle() /var/www/gi/artisan:36 PHP 3. Symfony\Component\Console\Application->run() /var/www/gi/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:107 PHP 4. Symfony\Component\Console\Application->doRun() /var/www/gi/vendor/symfony/console/Application.php:119 PHP 5. Symfony\Component\Console\Application->doRunCommand() /var/www/gi/vendor/symfony/console/Application.php:188 PHP 6. Illuminate\Console\Command->run() /var/www/gi/vendor/symfony/console/Application.php:843 PHP 7. Symfony\Component\Console\Command\Command->run() /var/www/gi/vendor/laravel/framework/src/Illuminate/Console/Command.php:136 PHP 8. Illuminate\Console\Command->execute() /var/www/gi/vendor/symfony/console/Command/Command.php:261 PHP 9. Illuminate\Container\Container->call() /var/www/gi/vendor/laravel/framework/src/Illuminate/Console/Command.php:150 PHP 10. call_user_func_array:{/var/www/gi/vendor/laravel/framework/src/Illuminate/Container/Container.php:507}() /var/www/gi/vendor/laravel/framework/src/Illuminate/Container/Container.php:507 PHP 11. System\Console\PluginRefresh->fire() /var/www/gi/vendor/laravel/framework/src/Illuminate/Container/Container.php:507 PHP 12. System\Classes\UpdateManager->updatePlugin() /var/www/gi/modules/system/Console/PluginRefresh.php:54 PHP 13. System\Classes\VersionManager->updatePlugin() /var/www/gi/modules/system/Classes/UpdateManager.php:470 PHP 14. System\Classes\VersionManager->applyPluginUpdate() /var/www/gi/modules/system/Classes/VersionManager.php:94 PHP 15. System\Classes\VersionManager->applyDatabaseScript() /var/www/gi/modules/system/Classes/VersionManager.php:141 PHP 16. October\Rain\Database\Updater->setUp() /var/www/gi/modules/system/Classes/VersionManager.php:392 PHP 17. October\Rain\Database\Updater->resolve() /var/www/gi/vendor/october/rain/src/Database/Updater.php:22 PHP 18. October\Rain\Database\Updater->getClassFromFile() /var/www/gi/vendor/october/rain/src/Database/Updater.php:74 PHP 19. token_get_all() /var/www/gi/vendor/october/rain/src/Database/Updater.php:110

And the migration code

<?php

namespace NotPaper\GidesignCms\Updates;

use Schema;
use October\Rain\Database\Updates\Migration;

/**
 * Class BuilderTableUpdateNotpaperGidesigncmsBanners3
 * @package NotPaper\GidesignCms\Updates
 */
class BuilderTableUpdateNotpaperGidesigncmsBanners3 extends Migration
{
    /**
     * Drop column banner.
     */
    public function up()
    {
        Schema::table('notpaper_gidesigncms_banners', function($table)
        {
            $table->dropColumn('banner');
        });
    }

    /**
     * Add column banner.
     */
    public function down()
    {
        Schema::table('notpaper_gidesigncms_banners', function($table)
        {
            $table->string('banner', 255);
        });
    }
}

Reproduce steps

Use the builder to add migrations and add phpDoc block. Then it gives this warning.

October build

Sorry, can't find the build number.

[LukeTowers]

I've experienced this myself a couple of times, seems very random. I believe there's already at least one issue open for this, but it may have been closed due to lack of ongoing discussion. As I recall, manually recreating the migration fixed it in my case, but I could not tell what the difference between the two files was.

If you could work on revising your migrations manually in your code editor until they work and then help us nail down what's causing these issues, that would be greatly appreciated.

[getrightnl]

I know i've seen the issue (saw it was closed) and referenced it in my issue. It is a really strange issue, but I'm glad to help. Could it be a problem with the different line endings that the builder builds. And could not be parsed by OctoberCMS

If I'm not mistaken the builder outputs different line endings. If I push with git I get a line ending warning which converts it to a different line ending.

[getrightnl]

To be specific this is the warning I get from Git.

warning: CRLF will be replaced by LF in plugins/notpaper/gidesigncms/updates/builder_table_update_notpaper_gidesigncms_clients.php.

[LukeTowers]

So if you replace the line endings yourself, does that fix the issue with that file?

[intheweb]

Could this be related to https://github.com/octobercms/october/issues/2766 ?

[LukeTowers]

I don't think so @intheweb, when I was encountering it it was happening regardless of the line endings being all of one or the other or mixed. Do try to replicate it yourself though and let me know what you discover.

[jpruiz114]

Hello. I'm getting the same error. Do we know why is this happening?

[jpruiz114]

PHP Warning:  Unterminated comment starting line 12 in /home/vagrant/Code/leadlava/vendor/october/rain/src/Database/Updater.php on line 110
PHP Stack trace:
PHP   1. {main}() /home/vagrant/Code/leadlava/artisan:0
PHP   2. Illuminate\Foundation\Console\Kernel->handle() /home/vagrant/Code/leadlava/artisan:35
PHP   3. Symfony\Component\Console\Application->run() /home/vagrant/Code/leadlava/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:107
PHP   4. Symfony\Component\Console\Application->doRun() /home/vagrant/Code/leadlava/vendor/symfony/console/Application.php:119
PHP   5. Symfony\Component\Console\Application->doRunCommand() /home/vagrant/Code/leadlava/vendor/symfony/console/Application.php:188
PHP   6. Illuminate\Console\Command->run() /home/vagrant/Code/leadlava/vendor/symfony/console/Application.php:843
PHP   7. Symfony\Component\Console\Command\Command->run() /home/vagrant/Code/leadlava/vendor/laravel/framework/src/Illuminate/Console/Command.php:136
PHP   8. Illuminate\Console\Command->execute() /home/vagrant/Code/leadlava/vendor/symfony/console/Command/Command.php:261
PHP   9. Illuminate\Container\Container->call() /home/vagrant/Code/leadlava/vendor/laravel/framework/src/Illuminate/Console/Command.php:150
PHP  10. call_user_func_array:{/home/vagrant/Code/leadlava/vendor/laravel/framework/src/Illuminate/Container/Container.php:507}() /home/vagrant/Code/leadlava/vendor/laravel/framework/src/Illuminate/Container/Container.php:507
PHP  11. System\Console\OctoberUp->fire() /home/vagrant/Code/leadlava/vendor/laravel/framework/src/Illuminate/Container/Container.php:507
PHP  12. System\Classes\UpdateManager->update() /home/vagrant/Code/leadlava/modules/system/console/OctoberUp.php:42
PHP  13. System\Classes\UpdateManager->updatePlugin() /home/vagrant/Code/leadlava/modules/system/classes/UpdateManager.php:141
PHP  14. System\Classes\VersionManager->updatePlugin() /home/vagrant/Code/leadlava/modules/system/classes/UpdateManager.php:470
PHP  15. System\Classes\VersionManager->applyPluginUpdate() /home/vagrant/Code/leadlava/modules/system/classes/VersionManager.php:94
PHP  16. System\Classes\VersionManager->applyDatabaseScript() /home/vagrant/Code/leadlava/modules/system/classes/VersionManager.php:141 PHP  17. October\Rain\Database\Updater->setUp() /home/vagrant/Code/leadlava/modules/system/classes/VersionManager.php:392
PHP  18. October\Rain\Database\Updater->resolve() /home/vagrant/Code/leadlava/vendor/october/rain/src/Database/Updater.php:22
PHP  19. October\Rain\Database\Updater->getClassFromFile() /home/vagrant/Code/leadlava/vendor/october/rain/src/Database/Updater.php:74
PHP  20. token_get_all() /home/vagrant/Code/leadlava/vendor/october/rain/src/Database/Updater.php:110

[LukeTowers]

Not yet, haven't had the time to look into it deeply. It may be the type of line endings within the file that gets generated. Are you able to replace your line endings to make them all unix or all MS and then retry and let us know how that goes?

[daftspunk]

The exception states it has an issue with the following code block

    /** <-- Line 25
     * Add column banner.
     */

This doesn't make much sense.

[LukeTowers]

That's what's happened to me a few times. Remove the comment blocks and it works somehow.

[daftspunk]

OH! I know why this happens! The Updater is trying to extract the class name from the file contents, but it only reads the first 512 bytes... this would occasionally cause a comment block to appear "unterminated".

See: https://github.com/octobercms/library/blob/master/src/Database/Updater.php#L109

getClassFromFile method:

$buffer .= fread($fileParser, 512);

[LukeTowers]

How can we fix this? Excellent catch by the way

[daftspunk]

Good question, the problem exists on the October website too when publishing a plugin (where the Plugin.php file has comments in it). See #537

My first attempt would be to suppress the warning, since the token parser is being a bit too strict about it, we don't care about the comments.

The whole point of this process is to analyse the file, without executing it, and not load the entire contents in to memory for performance reasons (imagine if the file was 20MB).

[daftspunk]

Another idea, this should close any comments that are unterminated, right?

$tokens = token_get_all($buffer . '/**/');

[LukeTowers]

Clever solution, that would indeed. I love it when simple solutions present themselves. We'd just need to add an inline comment to explain the extra '/**/' for future reference.

[getrightnl]

Ah makes sense! If the function stops in middle of comment it is unterminated of course! That would work, would close still open comment or just add a empty one.

Another solution could be to count all opening tags /* and count closing tags */ if they are not same add a */ like you said

[daftspunk]

We need someone to test that this fix works, I haven't actually been able to replicate it....

The code could actually read like this:

// Prefix and suffix string to prevent unterminated comment warning
$tokens = token_get_all('/**/' . $buffer . '/**/');

File located here: /vendor/october/rain/src/Database/Updater.php
Line: 109

I tried to produce the warning with this, but I receive no warnings:

token_get_all('$foo = "bar"; /* hello');

[getrightnl]

I can take a look at it end of this week probably. But what is the used workflow to get this bugfix approved by OctoberCMS? Can you send me some details about this

[daftspunk]

We just need you to tell us that it works 😄

[getrightnl]

Haha ok! 👍🏻

[getrightnl]

@daftspunk @LukeTowers added your code, tested and working on a file that was giving warnings:

// Prefix and suffix string to prevent unterminated comment warning
$tokens = token_get_all('/**/' . $buffer . '/**/');

[JunaidQadirB]

I got this today when I commented out a section of code in on of my migration's Schema::table function.

@getrightnl's patch is not in the current build. It fixed the error for me as well.

O CMS Build: 419
OS: Ubuntu Gnome 17.04