zordius / lightncandy

An extremely fast PHP implementation of handlebars ( http://handlebarsjs.com/ ) and mustache ( http://mustache.github.io/ ),
https://zordius.github.io/HandlebarsCookbook/
MIT License
608 stars 76 forks source link

Deprecation Warning on Lightncandy::prepare() #324

Closed danmcadams closed 4 years ago

danmcadams commented 4 years ago

The PHP Code:

    /**
     * Get a working render function by a string of PHP code. This method may requires php setting allow_url_include=1 and allow_url_fopen=1 , or access right to tmp file system.
     *
     * @param string      $php PHP code
     * @param string|null $tmpDir Optional, change temp directory for php include file saved by prepare() when cannot include PHP code with data:// format.
     * @param boolean     $delete Optional, delete temp php file when set to tru. Default is true, set it to false for debug propose
     *
     * @return Closure|false result of include()
     *
     * @deprecated
     */
    public static function prepare($php, $tmpDir = null, $delete = true)
    {
        $php = "<?php $php ?>";
        if (!ini_get('allow_url_include') || !ini_get('allow_url_fopen')) {
            if (!is_string($tmpDir) || !is_dir($tmpDir)) {
                $tmpDir = sys_get_temp_dir();
            }
        }

The Issue:

It doesn't seem that this method is actually deprecated as it's still the go-to function in the documentation, there's no "use this instead.", and I can't seem to find a replacement anywhere. IDEs complain about this warning and I believe it's confusing.

How about we get rid of the @deprecated annotation from this method?

Also, the @return Closure|false is a problem as closure isn't a valid return type here. I recommend changing to @return callable|bool

This prevents having to put an annotation above every Lightncandy::prepare($php) such as /** @var callable $renderer */

I've made a pull request here

reedy commented 4 years ago

PR closed, should this be too?