Fallback TV render can cause manager page to crash

[smg6511]

Bug Report / RFC

Summary

This is a bit of an edge-case problem I discovered when switching between a couple PR branches I've been working on. When TVs are rendered to a Resource editing form, there is a built-in fallback to the Text type if for some reason the requested TV render class can not be found. For quick reference, below is the relevant block in the getRender method of modTemplateVar (~L460-L477):

[...]
if ($className = $this->checkForRegisteredRenderMethod($type, $method)) {
    /** @var modTemplateVarOutputRender $render */
    $render = new $className($this);
    $output = $render->render($value, $params);
} else {
    $render = null;
    foreach ($paths as $path) {
        $renderFile = $path . $type . '.class.php';
        if (file_exists($renderFile)) {
            $className = include $renderFile;
            $logUnregisteredNames[] = $className;
            $this->registerRenderMethod($type, $method, $className);
            if (class_exists($className)) {
                /** @var modTemplateVarOutputRender $render */
                $render = new $className($this);
            }
            break;
        }
    }
   [...]   

Note how in the snippet above, the $className is fetched from the TV render file; each render file defines the TV type's render class and returns the render class name. This actually turns out to be a problematic pattern, as when a Resource has at least one Text TV and an attempt is made to create a fallback field for a missing TV render, a fatal error occurs because the Text render class has already been defined.

Another issue with the design here is that, when a fallback field is successfully rendered, it is done so completely silently. An error should be logged with clear explanation IMO.

Steps to Reproduce

1. Create a Template with at least one Text TV as well as another type (Textarea for example). I believe the order may matter here, so place your Textarea TV before the Text one.
2. Create a Resource with the above Template.
3. To force the condition of a missing render class, rename the Textarea render file, e.g.: core/src/Revolution/Processors/Element/TemplateVar/Renders/mgr/input/_textarea.class.php
4. Try to reload the Resource you just saved. You should see a 500 error.

Observed Behavior

(Noted in summary above.)

Possible Solutions

The quickest remedy would be to simply wrap the class definition in the Text render file (text.class.php) with a !class_exists condition, along with a clear comment in the file as to why that condition has been added directly in the render file. This is more of a band aid solution IMO.

A somewhat better remedy may be to call registerRenderMethod when the fallback is created (~L484-494) to ensure the attempt to load the Text class twice does not occur.

A less error-prone way would be to rethink relying on the renders' class definitions to return their class names. I believe these are the kind of side effects you'd usually want to avoid. It seems to me that the class names can be reliably derived from their method and type, with some small tweaks (namely changing class names such as modTemplateVarInputRenderTextArea to modTemplateVarInputRenderTextarea [Textarea being title case instead of camel case]), i.e.: $className = 'modTemplateVar' . ucfirst($method) . 'Render' . ucfirst($type); (I realize the treatment of $type above would need to be more fine-tuned to remove non-word characters, etc.)

I'll issue a PR once I get some opinion on the solutions above (or one I'm not thinking of).

Environment

MODX 3 / PHP 7.4

[Jako]

Would $className = include_once $renderFile; help here? If the file is already included, the result of include_once is true. Can the result be used to tell a difference?

[smg6511]

@Jako - I see what you're thinking, but, no — because the way it is currently designed, the class name is derived from what gets returned from each render file (meaning they need to be included every single time). I think that's a bit of a design flaw (which I allude to in my 3rd proposed solution).

[Jako]

Then I suggest an additional fallback renderer (which somehow can extend the textarea renderer). Since removing the classname return value is a breaking change and should go into Version 4.

[smg6511]

FYI: Changed from bug to proposal, as the basic issue has recently been fixed in another way.