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

Fixes #225: PHAR package support (second try) #261

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@resurtm
Contributor

resurtm commented May 14, 2013

#246 has been enhanced and improved.

Fixes #225.
Ready to be merged.

@qiangxue

View changes

Show outdated Hide outdated yii/helpers/base/FileHelper.php
*/
public static function realpath($path, $checkPhar = false)
{
if ($checkPhar && \Phar::running() === '') {

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member
if (!$checkPhar || \Phar::running() === '') {
@qiangxue

qiangxue May 14, 2013

Member
if (!$checkPhar || \Phar::running() === '') {

This comment has been minimized.

@resurtm

resurtm May 14, 2013

Contributor

Fixed.

@resurtm

resurtm May 14, 2013

Contributor

Fixed.

@qiangxue

View changes

Show outdated Hide outdated yii/web/CaptchaAction.php
@@ -85,7 +85,7 @@ class CaptchaAction extends Action
/**
* @var string the TrueType font file. This can be either a file path or path alias.
*/
public $fontFile = '@yii/web/SpicyRice.ttf';

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

This line is fine.

@qiangxue

qiangxue May 14, 2013

Member

This line is fine.

This comment has been minimized.

@resurtm

resurtm May 14, 2013

Contributor

Right, fixed.

@resurtm

resurtm May 14, 2013

Contributor

Right, fixed.

@qiangxue

View changes

Show outdated Hide outdated yii/web/CaptchaAction.php
} else {
$this->fontFile = Yii::$app->getRuntimePath() . '/SpicyRice.ttf';
if (!is_file($this->fontFile)) {
copy(\Phar::running(true) . '/web/SpicyRice.ttf', $this->fontFile);

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

I don't quite like this. Does it mean every core file who wants to deal with local files needs such line?

@qiangxue

qiangxue May 14, 2013

Member

I don't quite like this. Does it mean every core file who wants to deal with local files needs such line?

This comment has been minimized.

@resurtm

resurtm May 14, 2013

Contributor

Not at all. This is just a special case (peculiarity of the GD and maybe ImageMagick). Almost 99% of other asset, resource and standard files works well. file_get_contents, fopen and so forth could handle files inside PHARs without any problems.

I would be very happy if imagettfbbox() would allow to specify binary font data instead of bare TrueType font file path. But unfortunately this is not the case and there is only one (idiotic) workaround.

@resurtm

resurtm May 14, 2013

Contributor

Not at all. This is just a special case (peculiarity of the GD and maybe ImageMagick). Almost 99% of other asset, resource and standard files works well. file_get_contents, fopen and so forth could handle files inside PHARs without any problems.

I would be very happy if imagettfbbox() would allow to specify binary font data instead of bare TrueType font file path. But unfortunately this is not the case and there is only one (idiotic) workaround.

This comment has been minimized.

@resurtm

resurtm May 14, 2013

Contributor

Not quite sure but what do you think about the idea of converting TTFs into a regular asset files? Would be a bit sleeker in my opinion. Yes i realize that asset manager made for other purposes (publishing web accessible files in general) but after all it seems a better way.

@resurtm

resurtm May 14, 2013

Contributor

Not quite sure but what do you think about the idea of converting TTFs into a regular asset files? Would be a bit sleeker in my opinion. Yes i realize that asset manager made for other purposes (publishing web accessible files in general) but after all it seems a better way.

array_splice($path, $key - ($keyPosition * 2 + 1), 2);
}
$path = $scheme . str_replace('./', '', implode('/', $path));
return is_file($path) || is_dir($path) ? $path : false;

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

$path should use DIRECTORY_SEPARATOR rather than /. Otherwise you are changing the behavior of realpath() and will cause unexpected problems.

@qiangxue

qiangxue May 14, 2013

Member

$path should use DIRECTORY_SEPARATOR rather than /. Otherwise you are changing the behavior of realpath() and will cause unexpected problems.

@@ -78,7 +78,7 @@ public function init()
} elseif (!is_writable($this->basePath)) {
throw new InvalidConfigException("The directory is not writable by the Web process: {$this->basePath}");
} else {
$this->basePath = realpath($this->basePath);
$this->basePath = FileHelper::realpath($this->basePath);

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

FileHelper::realpath($this->basePath, true) ?
Actually why not changing the default value of $checkPhar to be true in FileHelper?

@qiangxue

qiangxue May 14, 2013

Member

FileHelper::realpath($this->basePath, true) ?
Actually why not changing the default value of $checkPhar to be true in FileHelper?

@@ -171,7 +173,7 @@ public function argumentsToString($args)
public function isCoreCode($trace)
{
if (isset($trace['file'])) {
return $trace['file'] === 'unknown' || strpos(realpath($trace['file']), YII_PATH . DIRECTORY_SEPARATOR) === 0;
return $trace['file'] === 'unknown' || strpos(FileHelper::realpath($trace['file'], true), YII_PATH) === 0;

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

The new code is not equivalent to the old one. Note that YII_PATH needs to be appended with DIRECTORY_SEPARATOR.
Also note that if FileHelper::realpath() always uses / as separator, the code may not work as expected since YII_PATH uses DIRECTORY_SEPARATOR as the separators.

@qiangxue

qiangxue May 14, 2013

Member

The new code is not equivalent to the old one. Note that YII_PATH needs to be appended with DIRECTORY_SEPARATOR.
Also note that if FileHelper::realpath() always uses / as separator, the code may not work as expected since YII_PATH uses DIRECTORY_SEPARATOR as the separators.

@@ -102,7 +102,14 @@ class CaptchaAction extends Action
*/
public function init()
{
$this->fontFile = Yii::getAlias($this->fontFile);

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

This line is needed to convert path alias to path.

@qiangxue

qiangxue May 14, 2013

Member

This line is needed to convert path alias to path.

if (\Phar::running() === '') {
$this->fontFile = Yii::getAlias($this->fontFile);
} else {
$this->fontFile = Yii::$app->getRuntimePath() . '/SpicyRice.ttf';

This comment has been minimized.

@qiangxue

qiangxue May 14, 2013

Member

What will be the result of Yii::getAlias($this->fontFile) in this case? Why do we need to hardcode the file name SpicyRice.ttf here?

@qiangxue

qiangxue May 14, 2013

Member

What will be the result of Yii::getAlias($this->fontFile) in this case? Why do we need to hardcode the file name SpicyRice.ttf here?

@qiangxue qiangxue referenced this pull request Oct 24, 2013

Merged

Validator message format #1051

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 23, 2013

Member

Closed for now. We may revisit this after GA.

Member

qiangxue commented Dec 23, 2013

Closed for now. We may revisit this after GA.

@qiangxue qiangxue closed this Dec 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment