Skip to content

Add namespaced aliases#2484

Merged
fabpot merged 2 commits intotwigphp:1.xfrom
nicolas-grekas:ns
May 25, 2017
Merged

Add namespaced aliases#2484
fabpot merged 2 commits intotwigphp:1.xfrom
nicolas-grekas:ns

Conversation

@nicolas-grekas
Copy link
Contributor

No description provided.

@nicolas-grekas
Copy link
Contributor Author

First commit bash+manual, second one automated with this:

<?php

$srcDir = __DIR__.'/src/';
 
foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($srcDir,  RecursiveDirectoryIterator::SKIP_DOTS), RecursiveIteratorIterator::LEAVES_ONLY) as $newFile) {

    $class = substr($newFile, strlen($srcDir), -4);

    $oldClass = 'Twig_'.strtr(substr(file_get_contents($newFile), 2, -5), '/', '_');
    $newClass = 'Twig\\'.strtr($class, '/', '\\');
    $oldFile = '/lib/'.strtr($oldClass, '_', '/').'.php';

    $ns = substr($newClass, 0, strrpos($newClass, '\\'));
    $newClassBase = substr($newClass, 1 + strrpos($newClass, '\\'));
    $dots = str_repeat('/..', substr_count($newClass, '\\'));

    file_put_contents($newFile, <<<EOTXT
<?php

namespace $ns;

require __DIR__.'$dots$oldFile';

if (\\false) {
    class $newClassBase extends \\$oldClass
    {
    }
}

EOTXT
    );

    file_put_contents(__DIR__.$oldFile, <<<EOTXT

class_alias('$oldClass', '$newClass', false);

EOTXT
        , FILE_APPEND
    );
}

@nicolas-grekas nicolas-grekas force-pushed the ns branch 2 times, most recently from b78d5c5 to a56b450 Compare May 24, 2017 18:47
@GromNaN
Copy link
Contributor

GromNaN commented May 24, 2017

Great move. I don't understand why we need to create an alias and a subclass for each existing class ?
Also, what is this trick with if (\false) {} ?

@nicolas-grekas
Copy link
Contributor Author

That's a good question :)
This is required to have Composer generate its class maps!

}
}

class_alias('Twig_Node_SetTemp', 'Twig\Node\SetTempNode', false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just wondering: this class has been removed in 2.x
why? missing deprecation notice in 1.x or mistake in 2.x?

Copy link
Member

Choose a reason for hiding this comment

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

I would say, missing @internal in 1.x

*/

/**
* @internal
Copy link
Contributor Author

@nicolas-grekas nicolas-grekas May 25, 2017

Choose a reason for hiding this comment

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

@stof
Copy link
Member

stof commented May 25, 2017

@GromNaN using a fully qualified name to access false is to allow resolving the condition at compile time (and so let OPCache drop dead code), instead of going through the fallback logic to determine whether a constant has been defined to overwrite false (yes PHP 5.x allows such thing)

@mindplay-dk
Copy link

Wouldn't it be simpler to just add a files entry to autoload in composer.json and define all the class-aliases there? It's one file hit, instead of one per class. Just wondering :-)

@Tobion
Copy link
Contributor

Tobion commented May 25, 2017

whether a constant has been defined to overwrite false

@stof How is that possible? I couldn't find a way to do so. I get one of these errors.

Fatal error: Cannot redeclare constant 'false'
Notice: Constant false already defined

@Tobion
Copy link
Contributor

Tobion commented May 25, 2017

Nvm, I got it:

namespace Foo;
define('Foo\false', true);
var_dump(false);

@Tobion
Copy link
Contributor

Tobion commented May 25, 2017

Wouldn't it be better to add the new classes together with scalar typehints?

@nicolas-grekas
Copy link
Contributor Author

nicolas-grekas commented May 25, 2017

@mindplay-dk the heavy call is the autoloader, not the file hit (when opcache is activated of course), so I prefer doing it this way because it makes it easy to then move gradually to using namespaces internally.

@Tobion I prefer keeping one topic per PR. This one is on branch 1.x btw, so php5.3 mini.

@stof
Copy link
Member

stof commented May 25, 2017

Thus, using files to register all aliases would be much more costly: this file would be included in all requests (even when not using Twig), and would trigger autoloading of all Twig classes (due to the target of the alias), even when you don't use most of them (in prod, most Twig classes are not used at runtime as templates are cached and lots of classes are related to the parser and the compiler)

@fabpot
Copy link
Contributor

fabpot commented May 25, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 5420a5e into twigphp:1.x May 25, 2017
fabpot added a commit that referenced this pull request May 25, 2017
This PR was squashed before being merged into the 1.x branch (closes #2484).

Discussion
----------

Add namespaced aliases

Commits
-------

5420a5e Add the aliases
71af32b Add namespaced aliases
fabpot added a commit that referenced this pull request May 25, 2017
This PR was merged into the 2.x branch.

Discussion
----------

[2.x] Add namespaced aliases

The 2.x-only part of #2484

Commits
-------

71265c4 [2.x] Add namespaced aliases
}
}

class_alias('Twig_SimpleFilter', 'Twig\TwigFilter', false);
Copy link
Member

Choose a reason for hiding this comment

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

Yeah for that. It gives us a clean migration path from Twig 1.x to 2.x to 3.x 😄

@nicolas-grekas nicolas-grekas deleted the ns branch May 27, 2017 11:07
@nicolas-grekas
Copy link
Contributor Author

Note that before tagging 1.34/2.4, we should make sure that we don't want to rename more classes. Doing it while adding the namespaced aliases makes it easy.
I'm thinking about e.g. Twig_Environment, which could be renamed Twig\Twig.
Trying to move Symfony to namespaced Twig before tagging could help spotting the cases where this might be useful.

@maidmaid
Copy link

Could you give an 3v4l example about if (\false)? I'm curious :)

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants