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

Blade cannot work properly, does not support custom directives/conditions #197

Closed
MarekGogol opened this issue Dec 21, 2018 · 3 comments
Closed

Comments

@MarekGogol
Copy link
Contributor

MarekGogol commented Dec 21, 2018

Hello,

thank you for you awesome package. I have som updates/fixes to your blade compiler.

If your application does have custom directives/conditions, your compiler does not work properly.

version: v4.6.1

Reproduction of error:

AppServiceProvider.app
In this provider we register our custom directive.

<?php

namespace App\Providers;

use Illuminate\Support\ServiceProvider;
use Illuminate\Support\Facades\Blade;

class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application services.
     *
     * @return void
     */
    public function register()
    {
        Blade::directive('title', function ($text) {
            return "<?php echo $text ?>";
        });
    }
}

layout.blade.php
My blade template with one working title and one buggy title... (Our application has custom directives for seo purposes...)

<h1>{{ _('This is my article, this title works OK') }}</h1>
<h2>@title( _('This is my second title, and this does not work properly') )</h2>

Solution:

Problem is in your blade extractor, where you are initializing new Illuminate\View\Compilers\BladeCompiler instance, instead of using existing facade Illuminate\Support\Facades\Blade which has registered all blade directives/conditions. When you initialize new BladeCompiler instance, then all custom directives binded into facade are missing. In this case your compiled template will not be properly compiled as in regular laravel response.

Solution number 1.
You can just simple use Facade for compiling blade template. Here is updated your blade compiler.

<?php

namespace Gettext\Extractors;

use Gettext\Translations;
use Illuminate\Support\Facades\Blade as BladeFacade;

/**
 * Class to get gettext strings from blade.php files returning arrays.
 */
class Blade extends Extractor implements ExtractorInterface
{
    /**
     * {@inheritdoc}
     */
    public static function fromString($string, Translations $translations, array $options = [])
    {
        $string = BladeFacade::compileString($string);

        PhpCode::fromString($string, $translations, $options);
    }
}

Solutions number 2
But I am not sure, why you are using this blade compiler instead of using facade. Maybe better solution for some users who is using Blade without laravel will be this. ( Maybe blade without laravel is 💩 :)), so we do not need this solutions. )

<?php

namespace Gettext\Extractors;

use Gettext\Translations;
use Illuminate\Filesystem\Filesystem;
use Illuminate\View\Compilers\BladeCompiler;

/**
 * Class to get gettext strings from blade.php files returning arrays.
 */
class Blade extends Extractor implements ExtractorInterface
{
    /**
     * {@inheritdoc}
     */
    public static function fromString($string, Translations $translations, array $options = [])
    {
        $cachePath = empty($options['cachePath']) ? sys_get_temp_dir() : $options['cachePath'];
        $bladeCompiler = new BladeCompiler(new Filesystem(), $cachePath);

        self::registerCustomDirectives($bladeCompiler);

        $string = $bladeCompiler->compileString($string);

        PhpCode::fromString($string, $translations, $options);
    }

    /*
     * Register custom directives
     */
    private static function registerCustomDirectives($bladeCompiler)
    {
        if ( !class_exists('\Illuminate\Support\Facades\Blade') )
            return;

        foreach (\Illuminate\Support\Facades\Blade::getCustomDirectives() as $key => $directive) {
            $bladeCompiler->directive($key, $directive);
        }
    }
}
@oscarotero
Copy link
Member

Hi.
Thanks for bring this up. The reason to do not use facades is because it's better for testing and code reusing.
Maybe we can add an option to use the facade. For example:

Blade::fromString($string, $translations, [
    'facade' => Illuminate\Support\Facades\Blade::class
]);

So, in the code:

    public static function fromString($string, Translations $translations, array $options = [])
    {
        if (empty($options['facade']) {
            $cachePath = empty($options['cachePath']) ? sys_get_temp_dir() : $options['cachePath'];
            $bladeCompiler = new BladeCompiler(new Filesystem(), $cachePath);
            $string = $bladeCompiler->compileString($string);
        } else {
            $string = $options['facade']::compileString($string);
        }

        PhpCode::fromString($string, $translations, $options);
    }

What do you think? Do you like to work in a pull request?

@MarekGogol
Copy link
Contributor Author

MarekGogol commented Dec 22, 2018

Yes, I like this option. I've created pull request, hoping we don't need tests for this, or?... But good idea will be highlight this option somewhere in docs for others who want correct exporting translates from directives.

@oscarotero
Copy link
Member

Yep, docs needs to be improved, not only for blade, but all other extractors/generators. But this is a different issue. Thanks for your contribution!

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

No branches or pull requests

2 participants