Image upload giving obscure error

[bobdenotter]

See screenshot:

I can be "fixed" by changing Field.php:

    private function shouldBeRenderedAsTwig($value): bool
    {
        try {
            $result = RequestZone::isForFrontend(Request::createFromGlobals()) && is_string($value) && $this->getDefinition()->get('allow_twig') && preg_match('/{[{%#]/', $value);
        } catch (\Exception $e) {
            $result = false;
        }

        return $result;
    }

Which is a terrible fix. So we should find out what's actually happening, and fix that.

[simongroenewolt]

Let me guess:

ContentHelper->get() has : string as return type.

preg_replace_callback at line 133 matches on line 168

if ($record->hasField($match[1])) {

Which returns $field

Field will now be 'toStringed' by php to match the return type of the get() function.

I'm guessing you were already at that point?

Now on Field.php the __toString() calls getTwigValue()which calls shouldBeRenderedAsTwig($value)

And shouldBeRenderedAsTwig($value) does this:

RequestZone::isForFrontend(Request::createFromGlobals())

I'm suspecting Request::createFromGlobals(): It tries to 'reconstruct' a request, if you've moved/deleted the original uploaded file and after that call this function you could very well get the file not found exception you show.

[bobdenotter]

It tries to 'reconstruct' a request, if you've moved/deleted the original uploaded file and after that call this function you could very well get the file not found exception you show.

Yes, I think that's exactly what happens! :-)

[bobdenotter]

Ah, i found it!!

We recently added this: https://github.com/bolt/redactor/blob/master/src/Controller/Upload.php#L110

Only now it tries to access the config after uploading, and then when it reconstructs the request, it's not working!

[simongroenewolt]

I saw resetting the $_FILES suggested as a 'fix' here laravel/framework#12350 (comment) -- feels wrong to me, put might work. Hope you can come up with a nicer solution.

[I-Valchev]

@bobdenotter can this be closed then, after the fixes in redactor and article?

[bobdenotter]

Kinda! I worked around it, with @simongroenewolt 's nasty hack, but it's not fixed per se.

The problem is in us using Request::createFromGlobals() instead of getting it injected in those places. But we use it, because we can't inject it in those places. Kinda chicken/egg thing.

So, it's less urgent than i assumed it was, but ideally we'd fix it properly.

[simongroenewolt]

I took another look at the code, it looks like the real solution would be to rework where the logic that decides on rendering 'as' twig or not is placed. To me it looks like it is too far down in the Field class at the moment. The solution should probably not be to bring the $request to the field, but to move the decision on how to render 'up'.

I noticed the Content class is used to pass a reference to the twig environment to the field while not using it itself. Also the field itself doing the RequestZone::isForFrontend() check feels a bit out-of-place to me.

I'm a bit stuck at the __call in Content.php though, as I cannot really see where that is used outside of the twig renderer.

@bobdenotter / @I-Valchev If you are interested I'd be happy to dig a bit further, but maybe you just want to leave this for now as and spend time elsewhere.