Some Magical Constants Don't Work in Cached Files

[loilo]

Problem: Using __DIR__ et al in config.php or DCA files obviously breaks when those files are cached in prod mode. There are ways around using such constants at all, but they are often pretty tedious (e.g. assuming the __DIR__ by constructing the path manually from TL_ROOT or the service container).

Suggested Solution: Since those cached files are lexed by the PhpFileLoader anyway, it should be easy to fix this behaviour by replacing the constants __DIR__, __FILE__ (and maybe, even if I don't see the use case there, __LINE__) with just primitive values.

Example: The original config.php at /var/www/contao/vendor/loilo/my-bundle/src/Resources/contao/config/config.php:

<?php
var_dump(
    __FILE__,
    __DIR__,
    __LINE__
);

would become the following in cache:

var_dump(
    "/var/www/contao/vendor/loilo/my-bundle/src/Resources/contao/config/config.php",
    "/var/www/contao/vendor/loilo/my-bundle/src/Resources/contao/config",
    5
);

Caveats: The only problem that I could see arising from replacing the constants would be somebody using them (for whatever weird reason) as parameters in function-like language constructs they are not allowed in (e.g. empty() in PHP < 5.5 or isset()).

If this is worth being considered, the primitives could also be extracted to some hashed constants defined at the beginning of the file (12345 being some file-related hash):

define("__FILE__12345", "/var/www/contao/vendor/loilo/my-bundle/src/Resources/contao/config/config.php");
define("__DIR__12345", "/var/www/contao/vendor/loilo/my-bundle/src/Resources/contao/config");
define("__LINE__12345", 5);

var_dump(
    __FILE__12345,
    __DIR__12345,
    __LINE__12345
);

Feedback: I don't think I'm the first developer to have this issue. Have you maybe already considered this but rejected the rather obvious solution due to other caveats I'm not aware of?

Otherwise, I'd be glad to attach a PR.

[fritzmg]

Why do you need those in DCA files, can you give an example use case?

[Toflar]

👎 on this. I'm pretty sure it will have side effects. I've never had to use that in almost ten years now, please provide a use case.

[loilo]

Use cases depend on what you'd consider legit things to do in those files. 🙃 I've used it sometimes in config.php as some kind of "execute this everytime Contao runs" hook, but probably not for things that really should have gone there.

However what brought me to finally filing this issue was the following case: It's not uncommon to have a tl_* class defined directly in a DCA file to manage behaviour of cut/paste logic etc. Now somebody on my team used __DIR__ inside this class to execute a binary in another directory of the bundle.

Heavily oversimplified, it looked like this:

$GLOBALS['TL_DCA']['tl_table'] = [
  // DCA stuff, including some callbacks à la [ 'tl_table', 'callbackHandler' ]
];

class tl_table {
    public function callbackHandler () {
        $neededForSomeDecision = exec(__DIR__ . '/../util/said-binary');

        // ...
    }
}

It's not like this would be hard to fix (by just sourcing the relevant parts out into their own classes). The rather critical point (which I definitely should have mentioned in the original issue description) is that this fails silently: The cache warmer does not care about __DIR__ , and so the binary execution, returning an empty string at run time, caused a non-trivial bug hunt. (Again, of course he should have checked if the execution was successful and otherwise check stderr and so on, but you're getting the point.)

IMHO Contao should act a little more defensive here and if those constants should not be supported, there should at least be something like a warning when they appear in files that are about to be cached.

[Toflar]

Code does not belong to configuration. It does not belong into config.php nor in any of the dca files. They are still there in Contao because of BC but it's bad practice. I don't see any need for action here at all :)

[loilo]

I agree with the configuration part. But from my experience — especially in PHP — this knowledge cannot be assumed for every developer, especially more junior ones. That's like relying on every car driver to always obey to the rules, it'll probably get you killed some day.

The analogy may ne be too suitable but I don't think some defensiveness would hurt here.

[Toflar]

The problem is, we cannot automatically fix it for them. You may add a PR that denies using these constants in these files tough, I'd like that :)

[leofeyer]

Why don't you use the absolute path to the binary?

[loilo]

@leofeyer Because that path differs from machine to machine. We don't have a Docker or Vagrant setup here but just some Apaches running on some Macs. What we did in the end was assembling the path with the help of Symfony's kernel.root_dir. However that was not the most satisfying experience since it also meant hardcoding the package's name in the DCA file.

That's not too bad and very unlikely to backfire, but still leaves that weird feeling of "If I'm gonna rename this someday, I'll 100% forget about that". Maybe I'm just having some slight OCD. 🙃

@toflar That's what I meant, probably some kind of warning when building the cache. I'm not too confident in denying it completely since I can't tell for sure it wouldn't break something. But after all, it's "just" the cache, right? :)

[fritzmg]

Why do yo need the package's name? Just use

\System::getContainer()->getParameter('kernel.project_dir')

[loilo]

Because the binary is inside the package.

EDIT: Clarification, in case we're not on the same side here: I'm referring to the Composer package containing the Symfony bundle.

[loilo]

However, that's not the point. I know how to solve that problem (or to not let it appear in the first place).

This is more about pointing out that there should probably either be an automatic fix or a hint/warning/error when a user's doing this kind of fauxpas.

[fritzmg]

Because the binary is inside the package.

Then you need this: https://getcomposer.org/doc/articles/vendor-binaries.md ;)

[loilo]

I know I know I knooow. 😁 Thanks anyway. ^^

[loilo]

Okay, back to topic.

You may add a PR that denies using these constants in these files tough, I'd like that :)

@Toflar I'd like to do that. Would you rather go for a warning or for a complete denial of building the cache?

[m-vo]

Ooor you invest your energy in advancing this idea of allowing config-only dca's which I liked alot: #438

[loilo]

Liking that as well, but that's not the amount of energy I can currently invest. 😉