modxcms / revolution

MODX Revolution - Content Management Framework
https://modx.com/
GNU General Public License v2.0
1.36k stars 529 forks source link

Enhance debugging of chunks #14159

Open pepebe opened 5 years ago

pepebe commented 5 years ago

Summary

Knowing what placeholders are available inside a chunk is more or less a gamble. You can read the documentation or study the code. Both methods are not very reliable and can be quite time-consuming.

I propose to add a generic placeholder to getChunk ([[+chunk.properties]]). It will contain all properties given to getChunk and can be output inside the chunk:

pdoMenu / wayfinder chunk:

<!-- [[+chunk.properties]] -->
<li>[[+menutitle]]<li>

Solution

Add a new placeholder to getChunk:

1872 - https://github.com/modxcms/revolution/blob/4beca05219ea5e3b4011ff54268dc54e8623163a/core/model/modx/modx.class.php

    public function getChunk($chunkName, array $properties= array ()) {
        // add debug placeholder here
       $properties['chunk']['properties'] = print_r($properties,true);
        //
        $output= '';
        if ($this->getParser()) {
            $chunk= $this->parser->getElement('modChunk', $chunkName);
            if ($chunk instanceof modChunk) {
                $chunk->setCacheable(false);
                $output= $chunk->process($properties);
            }
        }
        return $output;

Questions:

Regards, pepebe

Edit: I guess this should also be added to parseChunk()

mindeffects commented 5 years ago

Please also add chunk.name because it can take a hell of a time to find out which chunk created an output. Being able to put the name of a chunk into a comment line would help a lot – especially when a chunk gets renamed or is being duplicated.

Jako commented 5 years ago

Nice idea, but this placeholder generation should only be enabled with a system setting.

pepebe commented 5 years ago

@Jako Opt-in is always a good thing!

@mindeffects And chunk.id would also be a nice thing. I have written an issue about this: https://github.com/modxcms/revolution/issues/12510

mindeffects commented 5 years ago

And got a +10 from me! ;-)

pepebe commented 5 years ago

@mindeffects Zeros are cheap, but surprisingly effective ;)

Mark-H commented 5 years ago

I'd use placeholders instead of properties for the naming - just to keep things accurate - but I can't really argue against the idea.

Though I do wonder.. how come @mindeffects get to have 10 votes?! [/random]

mindeffects commented 5 years ago

Mayby, if you think „binary“, it is not so much anymore. 😜

pepebe commented 5 years ago

@Mark-H Good point!

Picking up on something Mark mentioned in the other thread ( https://github.com/modxcms/revolution/issues/12510 ), a generic prefix could also be a good idea:

[[+_self.placeholders]], [[+_self.id]], [[+_self.name]]
JoshuaLuckers commented 5 years ago

I suggest that [[+_self.placeholders]] should contain the names of the passed properties only since the values can be called the "regular" way.

alroniks commented 5 years ago

Excellent idea, but I don't like variant with an underscore. [[+self.placeholders]] looks more readable.

JoshuaLuckers commented 5 years ago

With underscore there is a lower risk of overwriting a property set by a user/developer.

alroniks commented 5 years ago

It doesn't matter because I can use custom placeholder with _ too. Better to log a warning when such placeholder used but keep these things simple. Any special symbol increases the complexity of MODX because you need to remember about it.

JoshuaLuckers commented 5 years ago

Then I suggest naming it this. People who work with objects are already familiar with $this.

mindeffects commented 5 years ago

My first idea was the same, too: Why use „self“ and not „this“? Let‘s use „this“!

pepebe commented 5 years ago

Yes, [[+this.placeholders]] is much more consistent with our experience and expectations.

I'll write the necessary changes and try my luck with a PR.

pepebe commented 5 years ago

I wonder how we could "protect" "this." from being used by other extensions. Is anyone aware of such a placeholder?

alroniks commented 5 years ago

I wonder how we could "protect" "this." from being used by other extensions. Is anyone aware of such a placeholder?

Log warn or error into the log.

sergant210 commented 3 years ago

Nice idea. But I would do it through a new modifier "print" to output arrays. Sometimes it can be useful for snippet's output when it returns an array.

[[+this:print]]   // It prints an array of the modChunk object (the placeholder "this" is obtained from the modChunk::toArray() method).
[[+this.placeholders:print]]
patrickatwsrn commented 2 years ago

It would be great if this would be a thing in 3.1.