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

RuntimeException in version 1.6.x #123

Closed
calcosta opened this issue Mar 7, 2023 · 18 comments · Fixed by #125 or #126
Closed

RuntimeException in version 1.6.x #123

calcosta opened this issue Mar 7, 2023 · 18 comments · Fixed by #125 or #126

Comments

@calcosta
Copy link

calcosta commented Mar 7, 2023

Just updated to version 1.6.

Shrinking several libraries now breaks with Error: Unclosed string at position: or Error: Unclosed regex pattern at position.

One example is jquery-ui.js (see https://pastebin.com/NwEXPtAb): breaks e.g. on line 12958

I added $this->echo('|error|'); before throw new \RuntimeException('Unclosed regex pattern at position: ' . $this->index); and removed the exception to get an idea where the error occurs. See https://pastebin.com/AX1XFgS3 (starts on line 1074).

Thanks for help!

@tedivm
Copy link
Member

tedivm commented Mar 7, 2023

Thanks for narrowing it down- honestly that is the hardest part of a lot of these bugs, just figuring out where it's breaking. Looking at the line in question I've got a good idea of what's happening, so once I'm done with work today I should be able to tackle it.

@manavo
Copy link

manavo commented Mar 8, 2023

Not sure if this helps (or if it's the same), but I was able to track down a similar exception with Error: Unclosed regex pattern at position to having the combination g/ but not as part of a regex: var value = currentRating/other;

Failing test:

    public function testUnclosedRegexExceptionDoesNotHappenWithVariablesEndingWithG()
    {
        $this->expectNotToPerformAssertions();
        \JShrink\Minifier::minify('var value = currentRating/other;');
    }

@tedivm
Copy link
Member

tedivm commented Mar 8, 2023

Test cases are always super super helpful! I normally put them directly into the test suite when I'm working on fixes.

@manavo
Copy link

manavo commented Mar 8, 2023

Yeah, I'd include it in a pull request normally, but haven't had a chance to dig into why this is happening. And then I saw an open PR and this issue, so wasn't sure if it's already mostly resolved 🙂

@tedivm
Copy link
Member

tedivm commented Mar 8, 2023

I think I'll be able to resolve it with about an hour of work, but unfortunately last night I had a migraine so I didn't have a chance.

@manavo
Copy link

manavo commented Mar 8, 2023

No worries at all! Thanks for looking into this as quickly as you are!

@attrib
Copy link

attrib commented Mar 9, 2023

I'm running into this issue as well.

Maybe my reduced test case helps when trying to test it:

<?php

require 'vendor/autoload.php';

// https://github.com/salesagility/SuiteCRM/blob/hotfix/themes/SuiteP/js/style.js#L183
var_dump(JShrink\Minifier::minify('if (spans[jj].className.match(/currentTab.*/)) {
    tabNum = ii;
}'));

Returns: Unclosed regex pattern at position: 49

I also tried to apply the fix from #124 but the issue remains the same.

@attrib
Copy link

attrib commented Mar 9, 2023

Actually noticed the above error is the error with the #124 applied.

Here an example when I do it without #124

<?php

require 'vendor/autoload.php';

var_dump(JShrink\Minifier::minify('if(D>(F/2){console.log("a")}'));

Works if I add spaces around the /, but doesn't minify it

<?php

require 'vendor/autoload.php';

var_dump(JShrink\Minifier::minify('if(D>(F / 2){console.log("a")}'));

Results in if(D>(F / 2){console.log("a")}
Expected: if(D>(F/2){console.log("a")}

Hope this helps while debugging the issue.

Downgrading to JShrink 1.5.0 for the meanwhile.

tedivm added a commit that referenced this issue Mar 9, 2023
@tedivm
Copy link
Member

tedivm commented Mar 9, 2023

A fix has been pushed up! I've incorporated all of the examples from this thread into the test suite as well to prevent future regressions.

Thanks for everyone's help!

@maciej-orba
Copy link

Hello

After using updated version number of issues is less but still exist (before hundreds, now few)!

ex.


Compilation from source /var/www/magento/lib/web/jquery/jquery-ui.js failed
RuntimeException: Unclosed regex pattern at position: 420835 in /var/www/magento/vendor/tedivm/jshrink/src/JShrink/Minifier.php:634
Stack trace:
#0 /var/www/magento/vendor/tedivm/jshrink/src/JShrink/Minifier.php(300): JShrink\Minifier->saveRegex()
#1 /var/www/magento/vendor/tedivm/jshrink/src/JShrink/Minifier.php(162): JShrink\Minifier->loop()
#2 /var/www/magento/vendor/tedivm/jshrink/src/JShrink/Minifier.php(137): JShrink\Minifier->minifyToString('/*! jQuery UI -...', Array)
#3 /var/www/magento/vendor/magento/framework/Code/Minifier/Adapter/Js/JShrink.php(27): JShrink\Minifier::minify('/*! jQuery UI -...')
#4 /var/www/magento/vendor/magento/framework/View/Asset/PreProcessor/Minify.php(60): Magento\Framework\Code\Minifier\Adapter\Js\JShrink->minify('/*! jQuery UI -...')
#5 /var/www/magento/vendor/magento/framework/View/Asset/PreProcessor/Pool.php(77): Magento\Framework\View\Asset\PreProcessor\Minify->process(Object(Magento\Framework\View\Asset\PreProcessor\Chain))
#6 /var/www/magento/vendor/magento/framework/View/Asset/Source.php(152): Magento\Framework\View\Asset\PreProcessor\Pool->process(Object(Magento\Framework\View\Asset\PreProcessor\Chain))
#7 /var/www/magento/vendor/magento/framework/View/Asset/Source.php(105): Magento\Framework\View\Asset\Source->preProcess(Object(Magento\Framework\View\Asset\File))
#8 /var/www/magento/vendor/magento/framework/View/Asset/File.php(159): Magento\Framework\View\Asset\Source->getFile(Object(Magento\Framework\View\Asset\File))
#9 /var/www/magento/vendor/magento/framework/App/View/Asset/Publisher.php(74): Magento\Framework\View\Asset\File->getSourceFile()
#10 /var/www/magento/vendor/magento/framework/App/View/Asset/Publisher.php(62): Magento\Framework\App\View\Asset\Publisher->publishAsset(Object(Magento\Framework\View\Asset\File))
#11 /var/www/magento/vendor/magento/module-deploy/Service/DeployStaticFile.php(97): Magento\Framework\App\View\Asset\Publisher->publish(Object(Magento\Framework\View\Asset\File))
#12 /var/www/magento/vendor/magento/module-deploy/Service/DeployPackage.php(189): Magento\Deploy\Service\DeployStaticFile->deployFile('jquery/jquery-u...', Array)
#13 /var/www/magento/vendor/magento/module-deploy/Service/DeployPackage.php(133): Magento\Deploy\Service\DeployPackage->processFile(Object(Magento\Deploy\Package\PackageFile), Object(Magento\Deploy\Package\Package))
#14 /var/www/magento/vendor/magento/module-deploy/Service/DeployPackage.php(102): Magento\Deploy\Service\DeployPackage->deployEmulated(Object(Magento\Deploy\Package\Package), Array, true)
#15 [internal function]: Magento\Deploy\Service\DeployPackage->Magento\Deploy\Service\{closure}()
#16 /var/www/magento/vendor/magento/framework/App/State.php(187): call_user_func_array(Object(Closure), Array)
#17 /var/www/magento/generated/code/Magento/Framework/App/State/Interceptor.php(68): Magento\Framework\App\State->emulateAreaCode('adminhtml', Object(Closure), Array)
#18 /var/www/magento/vendor/magento/module-deploy/Service/DeployPackage.php(103): Magento\Framework\App\State\Interceptor->emulateAreaCode('adminhtml', Object(Closure))
#19 /var/www/magento/vendor/magento/module-deploy/Process/Queue.php(354): Magento\Deploy\Service\DeployPackage->deploy(Object(Magento\Deploy\Package\Package), Array, true)
#20 /var/www/magento/vendor/magento/module-deploy/Process/Queue.php(267): Magento\Deploy\Process\Queue->execute(Object(Magento\Deploy\Package\Package))
#21 /var/www/magento/vendor/magento/module-deploy/Process/Queue.php(248): Magento\Deploy\Process\Queue->executePackage(Object(Magento\Deploy\Package\Package), 'adminhtml/Magen...', Array, false)
#22 /var/www/magento/vendor/magento/module-deploy/Process/Queue.php(176): Magento\Deploy\Process\Queue->assertAndExecute('adminhtml/Magen...', Array, Array)
#23 /var/www/magento/vendor/magento/module-deploy/Strategy/QuickDeploy.php(84): Magento\Deploy\Process\Queue->process()
#24 /var/www/magento/vendor/magento/module-deploy/Service/DeployStaticContent.php(115): Magento\Deploy\Strategy\QuickDeploy->deploy(Array)
#25 /var/www/magento/setup/src/Magento/Setup/Console/Command/DeployStaticContentCommand.php(136): Magento\Deploy\Service\DeployStaticContent->deploy(Array)
#26 /var/www/magento/vendor/symfony/console/Command/Command.php(298): Magento\Setup\Console\Command\DeployStaticContentCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#27 /var/www/magento/vendor/symfony/console/Application.php(1040): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#28 /var/www/magento/vendor/symfony/console/Application.php(301): Symfony\Component\Console\Application->doRunCommand(Object(Magento\Setup\Console\Command\DeployStaticContentCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#29 /var/www/magento/vendor/magento/framework/Console/Cli.php(116): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#30 /var/www/magento/vendor/symfony/console/Application.php(171): Magento\Framework\Console\Cli->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#31 /var/www/magento/bin/magento(23): Symfony\Component\Console\Application->run()

@maciej-orba
Copy link

@maciej-orba
Copy link

Please reopen

@tedivm
Copy link
Member

tedivm commented Mar 9, 2023

@maciej-orba - check out the latest PR, #126, it should resolve this. I've included the jquery-ui test in the test suite itself to confirm that no errors are being triggered.

@maciej-orba
Copy link

I confirm, Static Content Deploy for Magento works fine, with no issues on latest JShrink
Thank you!

adminhtml/Magento/backend/en_US         1825/2923           =================>---------- 62%    6 secs              
frontend/Magento/luma/en_US             1007/2199           ============>--------------- 45%    6 secs
frontend/Magento/blank/en_US            2102/2183           ==========================>- 96%    11 secs             
adminhtml/Magento/backend/en_US         2678/2923           =========================>-- 91%    11 secs             
frontend/Magento/luma/en_US             2099/2199           ==========================>- 95%    11 secs
frontend/Magento/blank/en_US            2183/2183           ============================ 100%   11 secs             
adminhtml/Magento/backend/en_US         2923/2923           ============================ 100%   11 secs             
frontend/Magento/luma/en_US             2199/2199           ============================ 100%   11 secs
Execution time: 14.963996887207

@tedivm
Copy link
Member

tedivm commented Mar 9, 2023

Thank you for confirming that- if you have any other issues please feel free to open up an issue here!

@hendralin
Copy link

Hello,

I'm still using version 1.6.0 and PHP7, but Error: Unclosed regex pattern at position...still appear.

because since 1.6.1 it dropped PHP 7 support.

is there anyway to patch this error?

I'm now downgrade to 1.5.0

Thank you

@tedivm
Copy link
Member

tedivm commented Mar 10, 2023

I should have dropped php7 support in 1.6 but didn't as an error. You should lock to 1.5 or even 1.4 if you're having trouble and want to stay on the no longer supported php7. You should really upgrade to php8 though, as php7 isn't getting security updates anymore.

@tedivm
Copy link
Member

tedivm commented Mar 10, 2023

I'm going to lock this topic for now- if people find their way here please just open up a new ticket with any issues you're having.

@tedious tedious locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants