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

Add namespaced aliases #2484

Merged
merged 2 commits into from May 25, 2017

Conversation

Projects
None yet
7 participants
@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented May 24, 2017

No description provided.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented May 24, 2017

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 nicolas-grekas:ns branch 2 times, most recently from b78d5c5 to a56b450 May 24, 2017

@GromNaN

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented May 24, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:ns branch from a56b450 to 7b9e305 May 25, 2017

@@ -33,3 +33,5 @@ public function compile(Twig_Compiler $compiler)
;
}
}
class_alias('Twig_Node_SetTemp', 'Twig\Node\SetTempNode', false);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 25, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@stof

stof May 25, 2017

Contributor

I would say, missing @internal in 1.x

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:ns branch from 7b9e305 to 5420a5e May 25, 2017

@@ -9,6 +9,9 @@
* file that was distributed with this source code.
*/
/**
* @internal

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 25, 2017

Author Contributor
@stof

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

mindplay-dk commented May 25, 2017

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

Tobion commented May 25, 2017

Nvm, I got it:

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

This comment has been minimized.

Copy link
Contributor

Tobion commented May 25, 2017

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

@nicolas-grekas

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

fabpot commented May 25, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 5420a5e into twigphp:1.x May 25, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 25, 2017

feature #2484 Add namespaced aliases (nicolas-grekas)
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

feature #2486 [2.x] Add namespaced aliases (nicolas-grekas)
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
@@ -117,3 +117,5 @@ public function getAlternative()
return $this->options['alternative'];
}
}
class_alias('Twig_SimpleFilter', 'Twig\TwigFilter', false);

This comment has been minimized.

Copy link
@stof

stof May 26, 2017

Contributor

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 nicolas-grekas:ns branch May 27, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented May 27, 2017

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

This comment has been minimized.

Copy link

maidmaid commented May 30, 2017

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
You can’t perform that action at this time.