Document the Twig conflict when an application loads its own Twig - #2688

I'm not sure I follow.. Is the problem you're trying to fix here that SimpleSAMLphp and your application use different versions of Twig?
Messing with files in the vendor-directory is never the solution

I'm not sure I follow.. Is the problem you're trying to fix here that SimpleSAMLphp and your application use different versions of Twig? Messing with files in the vendor-directory is never the solution

Not quite, it's two copies of Twig in the same PHP process, even with the same version. In the issue, the application loads Debian's Twig from /usr/share/php/Twig and the tarball brings its own in vendor/. Twig's src/Resources/*.php files declare global functions such as twig_cycle() without a function_exists() guard, so the second autoloader to include them fails with "Cannot redeclare". Twig won't change that in 3.x (twigphp/Twig#4180 was closed; the functions are removed in Twig 4).

I agree that editing vendor/ is not nice; I followed the solution proposed in #2687. If you prefer, I can drop the build change and instead document that an application with its own Twig should install SimpleSAMLphp with Composer rather than use the tarball, so only one Twig is loaded. @jornane, does that work for your setup?

If you prefer, I can drop the build change and instead document that an application with its own Twig should install SimpleSAMLphp with Composer rather than use the tarball, so only one Twig is loaded. @jornane, does that work for your setup?

To me this would be the preferred solution, yes. Either this, or you could run your application and SimpleSAMLphp in different PHP-FPM pools so they don't interfere?
Jorn is familiar with this project, so we should be able to work something out together without dirty hacks.

If you prefer, I can drop the build change and instead document that an application with its own Twig should install SimpleSAMLphp with Composer rather than use the tarball, so only one Twig is loaded. @jornane, does that work for your setup?

To me this would be the preferred solution, yes. Either this, or you could run your application and SimpleSAMLphp in different PHP-FPM pools so they don't interfere? Jorn is familiar with this project, so we should be able to work something out together without dirty hacks.

As suggested, I've dropped the build change and turned this into a documentation update: a short section in the SP integration docs about the "Cannot redeclare twig_cycle()" error, with both options, installing SimpleSAMLphp with Composer or running it in a separate PHP-FPM pool. @jornane, happy to adjust if your setup needs more.

Hi @Sharawey74, thank you for looking into this! For our application, the proposed solution does not help, but in any case I support documenting the issue!

We want to make our application available as tarball and Debian package, so the Composer solution will only work partially, and I still need to make a workaround for other cases. I think that the solution with two different PHP-FPM pools won't work, because the application needs to be able to interact with \SimpleSAML\Auth\Simple, at which point the autoloader will load the problematic files.

If we would allow making changes to vendor, it would probably be better to truncate the 4 problematic files than to patch the autoloader array.

Ideally, the problem would be fixed upstream, twigphp/Twig#4180, maybe we can ask them to reconsider and add function_exists guards?

In any case, I think I can make a workaround in our application if we can't fix it in SimpleSAMLphp or upstream. The documentation is good, but reconsider if the PHP-FPM recommendation can actually work.

P.S. The upstream Twig ticket seems to suggest patching Twig so it uses a different namespace, thus removing any conflict. I'd rather avoid that.

Hi @Sharawey74, thank you for looking into this! For our application, the proposed solution does not help, but in any case I support documenting the issue!

We want to make our application available as tarball and Debian package, so the Composer solution will only work partially, and I still need to make a workaround for other cases. I think that the solution with two different PHP-FPM pools won't work, because the application needs to be able to interact with \SimpleSAML\Auth\Simple, at which point the autoloader will load the problematic files.

If we would allow making changes to vendor, it would probably be better to truncate the 4 problematic files than to patch the autoloader array.

Ideally, the problem would be fixed upstream, twigphp/Twig#4180, maybe we can ask them to reconsider and add function_exists guards?

In any case, I think I can make a workaround in our application if we can't fix it in SimpleSAMLphp or upstream. The documentation is good, but reconsider if the PHP-FPM recommendation can actually work.

P.S. The upstream Twig ticket seems to suggest patching Twig so it uses a different namespace, thus removing any conflict. I'd rather avoid that.

Thanks @jornane, good point: if the application calls \SimpleSAML\Auth\Simple, both have to run in the same PHP process, and if it doesn’t, the conflict never happens, so the PHP-FPM option doesn’t actually help. I’ve removed it; the section now only recommends installing SimpleSAMLphp with Composer.

I did some more testing over the weekend, and got some interesting findings.

Regarding the Composer recommendation, we might change the recommendation a bit. If you install Twig using Composer, the tarball version of SimpleSAMLphp works. This is because of some global state variable that Composer sets, it stores the md5-hashes of the packages:paths of the files that have been included. Since the packages and paths are the same for the application and SimpleSAMLphp, this prevents the error from triggering.

This works:

• Install Twig via Composer
• Install SimpleSAMLphp from tarball
• Include the Composer autoloader for your application (optimized or not makes no difference); include 'vendor/autoload.php';
• Include the SimpleSAMLphp autoloader from tarball include 'simplesamlphp/lib/_autoload.php';
• new \SimpleSAML\Auth\Simple('default-sp');

This doesn't work:

• Install Twig via apt-get
• Install SimpleSAMLphp from anywhere (composer, tarball, Debian package)
• Include the autoloader for your application (doesn't matter what it is)
• Include the Debian-provided Twig autoloader include '/usr/share/php/Twig/autoload.php';
• Include the SimpleSAMLphp autoloader from tarball include 'simplesamlphp/lib/_autoload.php';
• Cannot redeclare function

Similar, if you installed SimpleSAMLphp through composer, and thus don't load the SimpleSAMLphp autoloader, you get the error when you load the application's autoloader or the Twig autoloader, whichever is loaded later.

The Composer-generated autoloader remembers state whether it included Twig, even if there's two different installations of the same version. I checked how I can write to this state so that these problematic files won't be included. I found this works reliably, from SimpleSAMLphp 2.3.0 up until the latest version (did not try earlier versions, I don't see a reason they won't work). Since the hashes are based on the path (and not file contents), they are stable.

// Set hashes to "true" to indicate that they have already been included
$GLOBALS['__composer_autoload_files']['89efb1254ef2d1c5d80096acd12c4098'] = // twig/twig:src/Resources/core.php
$GLOBALS['__composer_autoload_files']['ffecb95d45175fd40f75be8a23b34f90'] = // twig/twig:src/Resources/debug.php
$GLOBALS['__composer_autoload_files']['c7baa00073ee9c61edf148c51917cfb4'] = // twig/twig:src/Resources/escaper.php
$GLOBALS['__composer_autoload_files']['f844ccf1d25df8663951193c3fc307c8'] = // twig/twig:src/Resources/string_loader.php
true;

In order to have SimpleSAMLphp and application both use Twig, you'd either need to:

• Make sure that Twig is loaded through a Composer autoloader
• Set the global variable so that SimpleSAMLphp's Composer autoloader won't load the deprecated Twig files (either by using Composer for the application itself, or by manually setting the global variable)

Alternatively, these lines could be added to src/_autoload.php so that these files aren't loaded anyway.

So it looks like the updated PR for this should be about not installing Twig from system packages if you are using both?

@monkeyiq
So it looks like the updated PR for this should be about not installing Twig from system packages if you are using both?

That would mean that I'd have to go against recommendations, but that will fine as long as SimpleSAMLphp uses Composer, and ships the Composer autoloader in the tarball.

I was mainly looking at your comment above with "This works: / This doesn't work".

If the PR recommendation of "To avoid this, install SimpleSAMLphp as a Composer dependency of your application instead of using the release archive" is fine then I'll merge this.

I was mainly looking at your comment above with "This works: / This doesn't work".

If the PR recommendation of "To avoid this, install SimpleSAMLphp as a Composer dependency of your application instead of using the release archive" is fine then I'll merge this.

This is not correct. If you install SimpleSAMLphp as a Composer dependency of your application, but your application also uses Twig from apt-get, it will still fail.

My recommendation is to add mitigations so that Twig won't load the deprecated functions, since they are not used in SimpleSAMLphp. The original PR did this by making changes to Composer-generated files, but I have since learned that the same can be achieved by setting a variable prior to loading the autoloader.

I propose we do the latter, rather than just documenting "it doesn't work if you hold it this way". This would allow me to publish my application as a Debian package without going against SimpleSAMLphp recommendations. The mitigation can either be part of the SimpleSAMLphp code, or be mentioned in the documentation, so that the calling application can implement it.

Thanks @jornane for testing this. You're right: installing with Composer doesn't help when Twig comes from system packages, so the current wording is misleading.

Two options:

1. Add the $GLOBALS['__composer_autoload_files'] lines to src/_autoload.php, so the deprecated Twig files are always skipped.
2. Keep this docs-only, correct the wording, and document that workaround for applications to apply themselves.

@tvdijen @monkeyiq, which do you prefer? I'll update the PR either way.

I agree with @tvdijen that keeping this as a docs only update is the preferred path. Folks wanting to modify the source as shown in the other option will likely fine this PR and know of the other options if they wish to maintain changes out of tree.

Thanks @monkeyiq, docs-only it is. And thanks @jornane for the testing, it changed the advice completely.

I've rewritten the section:

• It no longer says installing with Composer fixes it. What matters is how the application loads Twig: through a Composer autoloader it works, from an OS package (like /usr/share/php/Twig/autoload.php) it fails however SimpleSAMLphp is installed.
• It documents the $GLOBALS['__composer_autoload_files'] workaround for applications that can't load Twig through Composer, with the four hashes. I checked that they match Composer's md5('twig/twig:src/Resources/<file>.php') keys in the current autoloader.

@tvdijen the content changed since your approval, so another look would be appreciated.

Thanks @Sharawey74! This way I don't have to modify SSP code and the fix is documented. I would change a word though, "it fails however SimpleSAMLphp is installed" -> "it fails regardless how SimpleSAMLphp is installed"

I agree with @tvdijen that keeping this as a docs only update is the preferred path. Folks wanting to modify the source as shown in the other option will likely fine this PR and know of the other options if they wish to maintain changes out of tree.

I'm not sure I understand what you mean with "maintain changes out of tree" in this context, but the solution as documented by @Sharawey74 is sufficient for my use case.