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

FileHelper::ClassInfo() regex not coping with windows newlines with XAMPP #1517

Closed
tskodaw opened this issue Sep 26, 2017 · 6 comments
Closed
Assignees
Labels
Milestone

Comments

@tskodaw
Copy link

tskodaw commented Sep 26, 2017

What steps will reproduce the problem?

  • have windows7 and xampp 7.1.8
  • get luya-kickstarter and deps from master via composer with min-stability : "dev"
  • apply config to config-files
  • on cli do php luya migrate
  • on cli do php luya import or php index.php import

What is the expected result?

correct program termination.

What do you get instead? (A Screenshot can help us a lot!)

F:\xampp7\htdocs\luyacomposertest\public_html>php index.php import
Exception 'ReflectionException' with message 'Class \AudioBlock does not exist'

in F:\xampp7\htdocs\luyacomposertest\vendor\yiisoft\yii2\di\Container.php:424

Stack trace:
#0 F:\xampp7\htdocs\luyacomposertest\vendor\yiisoft\yii2\di\Container.php(424): ReflectionClass->__construct('\\AudioBlock')
#1 F:\xampp7\htdocs\luyacomposertest\vendor\yiisoft\yii2\di\Container.php(364): yii\di\Container->getDependencies('\\AudioBlock')
#2 F:\xampp7\htdocs\luyacomposertest\vendor\yiisoft\yii2\di\Container.php(156): yii\di\Container->build('\\AudioBlock', Array, Array)
#3 F:\xampp7\htdocs\luyacomposertest\vendor\yiisoft\yii2\BaseYii.php(348): yii\di\Container->get('\\AudioBlock', Array, Array)
#4 F:\xampp7\htdocs\luyacomposertest\vendor\luyadev\luya-module-cms\src\admin\importers\BlockImporter.php(105): yii\BaseYii::createObject(Array)
#5 F:\xampp7\htdocs\luyacomposertest\vendor\luyadev\luya-module-cms\src\admin\importers\BlockImporter.php(63): luya\cms\admin\importers\BlockImporter->createBlockObject('\\AudioBlock')
#6 F:\xampp7\htdocs\luyacomposertest\vendor\luyadev\luya-module-cms\src\admin\importers\BlockImporter.php(92): luya\cms\admin\importers\BlockImporter->saveBlock('\\AudioBlock')
#7 F:\xampp7\htdocs\luyacomposertest\vendor\luyadev\luya-module-cms\src\admin\importers\BlockImporter.php(38): luya\cms\admin\importers\BlockImporter->saveBlockByPath('F:\\xampp7\\htdoc...')
#8 F:\xampp7\htdocs\luyacomposertest\vendor\luyadev\luya-core\console\commands\ImportController.php(157): luya\cms\admin\importers\BlockImporter->run()
[...]

Comment

basic problem:
in luya-core FileHelper::ClassInfo() line 82, the regex

if (preg_match('#^namespace\s+(.+?);$#sm', $phpCode, $m)) {

does not match windows newlines (CRLF), at least on my xampp-installation ,
changing it to:

if (preg_match('#^namespace\s+(.+?);\r?$#sm', $phpCode, $m)) {

solved the situation and worked fine on linux too,

additionally:
perhaps in luya-module-cms\BlockImporter::saveBlock
an error handling could be fine when BlockImporter::saveBlockByPath calls it with insufficient class name?

    {
        // ensure all classes start with trailing slash class name definition like `\foo\bar\Class`
        $fullClassName = '\\'  . ltrim($fullClassName, '\\');
        $model = Block::find()->where(['class' => $fullClassName])->one();
        
        $blockObject = $this->createBlockObject($fullClassName);
...

Additional infos

Q A
LUYA Version master-branch
PHP Version 7.1.8
Platform XAMPP 7.1.8
Operating system Windows7
@nadar nadar self-assigned this Sep 26, 2017
@nadar nadar added the bug label Sep 26, 2017
@nadar nadar added this to the 1.0.0 milestone Sep 26, 2017
@nadar
Copy link
Contributor

nadar commented Sep 26, 2017

thank you @tskodaw this is a very good input. I am going to add some test cases and add your fix thank you!

What would be an example for insufficient class name?

nadar added a commit that referenced this issue Sep 26, 2017
@nadar
Copy link
Contributor

nadar commented Sep 26, 2017

@tskodaw i just pushed a fix, could you please try again with the latest dev-master on luya-core?

@tskodaw
Copy link
Author

tskodaw commented Sep 26, 2017

@nadar works fine for me under win7 "Importer run successfully" :)

for BlockImporter::saveBlock($fullClassName) -> insufficient as testcase would be just \AudioBlock , without the namespace, which lead to the Exceptions

@nadar
Copy link
Contributor

nadar commented Sep 26, 2017

I think this could be dangerous as its possible to have such a namespace and is therefore not invalid.

Can we close your issue?

@tskodaw
Copy link
Author

tskodaw commented Sep 26, 2017

hmm good point,
perhaps in a future version handling the exception in saveBlock and saying Importer run unsuccessfull on class xyz please check you configuration at package abc

@tskodaw tskodaw closed this as completed Sep 26, 2017
@nadar
Copy link
Contributor

nadar commented Sep 26, 2017

I know what you mean. The problem is that every module or application can attache classes to the importer process as its very common routine, so therefore we can not stop, but maybe adding a hint "block importer has warning(s) XYZ" or sth like this. I am waiting for the yii2 console table output helper (which should released soon) then i will rewrite things anyway.

Thanks again for your report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants