Skip to content

Quote string for special characters. Fixes #3214 #13344

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

Closed
wants to merge 2 commits into from

Conversation

reillo
Copy link

@reillo reillo commented Jan 24, 2018

The path might contain regular expression character (i.e () which might cause UnexpectedValueException "Could not parse theme static file". Refer to #3214

Description

Fix issue of UnexpectedValueException "Could not parse theme static file" when 'bin/magento setup:static-content:deploy -f' is executed. This will also cause of missing static files.

Fixed Issues (if relevant)

  1. Deploying Static Files. Could not parse theme static file #3214: Deploying Static Files. Could not parse theme static file

Manual testing scenarios

  1. Put the Magento Application instance into the folder: C:/Program Files (x86)
  2. Run bin/magento setup:static-content:deploy -f

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

Windows 10 path might contain `(x64)` (i.e `Program Files (x 82)`)  which causes UnexpectedValueException. Refer to magento#3214
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jan 24, 2018

CLA assistant check
All committers have signed the CLA.

@miguelbalparda
Copy link
Contributor

miguelbalparda commented Jan 24, 2018

Windows is not supported as an operating system per DevDocs. Do you foresee any other cases where this might be relevant?

@reillo
Copy link
Author

reillo commented Jan 24, 2018

@miguelbalparda But it does work in Windows. That patch is not for windows only, it applies to all OS system. See the commit.

@reillo
Copy link
Author

reillo commented Jan 24, 2018

@magento-cicd2 CLA signed.

@@ -938,7 +938,7 @@ private function accumulateThemeStaticFiles($area, $locale, $filePattern, &$resu
$themePath . "/*_*/web/i18n/{$locale}"
];
$this->_accumulateFilesByPatterns($paths, $filePattern, $files);
$regex = '#^' . $themePath .
$regex = '#^' . preg_quote($themePath) .
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll also need to provide the second parameter to preg_quote to make sure the chose delimiter # gets escaped, too (so that the escape is always 100% correct).

Copy link
Author

Choose a reason for hiding this comment

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

Yep, right. Thanks @oliverklee

@miguelbalparda
Copy link
Contributor

miguelbalparda commented Jan 24, 2018

Your description only mentioned Windows, that's why I'm asking. I'm processing a similar PR and the same might apply.
To avoid any misunderstandings, while Magento 2 might work in Windows, it's not officially supported and that's why we usually reject PRs containing fixes for unsupported systems.
I'll check this internally and get back to you. Thanks!

@reillo
Copy link
Author

reillo commented Jan 24, 2018

@miguelbalparda Thanks, miguel. I'll be more specific next time. I'll update the description.

@orlangur
Copy link
Contributor

@reillo,

But it does work in Windows

Actually, not fully, there are some known issues. It was already discussed in some older issues that for good it would be nice to have a proper OS check similar to the PHP version one. Before such check is implemented Windows-only issues and pull requests can be simply closed with an explanation.

So, before any fix can be actually merged there should be a corresponding VALID bug scenario first.

@miguelbalparda miguelbalparda self-assigned this Jan 24, 2018
@miguelbalparda
Copy link
Contributor

To make things clear, @oliverklee is an independent developer who asked for changes in this PR.

@oliverklee
Copy link
Contributor

@miguelbalparda That's correct. :-)

As I'm quite new to the Magento project: Is it okay/helpful if I approve PRs that look okay to me, or should this task be limited to dedicated reviewers, core developers or something like this?

@miguelbalparda
Copy link
Contributor

I actually asked the same question today in the maintainers channel :)
Basically, feel free to ask for changes where you see fit and keep in mind the final decision is made by maintainers. Thanks!

@miguelbalparda
Copy link
Contributor

I'll close this for now, will revisit in the future if anything changes. Thanks @reillo!

@kanduvisla
Copy link
Contributor

Why isn't this fix applied? I'm trying to deploy from a Jenkins server (which runs on Linux), from with a Docker container (that is exactly the proper setup for Magento 2) and I'm also getting this error.

If I understand the issue correctly the issue is that I have parenthesis in my path /var/lib/jenkins/workspace/prik-en-tik/Job Name (acceptance)/vendor/magento/theme-frontend-blank/web/css/_styles.less. A simple preg_quote() fixes this. Heck, you should always preg_quote() variable input inside a regex!

I'm running into this issue in Magento 2.1.12 by the way. I vote to re-open this issue.

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.

6 participants