symfony-cmf/tree-browser-bundle

Context menu does not handle "all" option for valid children

benglass opened this issue · 17 comments

benglass commented

The context menu which should allow you to create new child nodes and delete nodes does not currently handle the configuration option where you set "all" in the sonata_doctrine_phpcr_admin.document_tree for valid children.

sonata_doctrine_phpcr_admin:
    document_tree:
        Symfony\Cmf\Bundle\SimpleCmsBundle\Doctrine\Phpcr\Page:
            valid_children:
                - all

This will result in NO 'Create' option appearing in the submenu, whereas what it should do is add all of the items in config.types to the submenu. This should be a relatively simple change in the init.js file.

This might be as simple as assigning valid_children to config.types if its value is "all" on line 107 (right before the the each loop)

valid_children = config.types[nodetype].valid_children === 'all' ? config.types : config.types[nodetype].valid_children;

Not sure if there would be some better way of letting this script know about the special all value but i think that would solve the issue.

benglass commented

Looks like its a little more complicated that my previous message suggested due to the format being different between config.types (hash table) and validchildren (array of class names). Additionally config.types contains an "undefined" option which we would not want to display. So something more like this would work, let me know if you have an alternate suggested approach or I can create a PR.

var i;
var validchildren = config.types[nodetype].valid_children;
if (validchildren.length === 1 && config.types[nodetype].valid_children[0] === 'all') {
    validchildren = [];
    for (type in config.types) {
        if (type === 'undefined') {
            continue;
        }
        validchildren.push(type);
    }
}
benglass commented

For the same reason you cannot drag and drop to move tree items unless you have explicitly configured the valid children (not using the "all" special value). Perhaps it would be best to normalize the document_tree config so that ["all"] was replaced with an array of all keys from config.types (possibly excluding "undefined" but im not sure if that is relevant to the tree browser?) before it was pass through to the javascript as it would solve both issues.

benglass commented

Also dragging and dropping to move items in the tree currently results in the functionality of editing them by clicking being broken because the path that is provided in the URL is no longer accurate (but has not been updated).

Example:
If you drag page /cms/simple/foo/bar so it is a sibling of foo instead of child (now /cms/simple/bar) when you click it still tries to load the edit link for the old path (/cms/simple/foo/bar) which no longer works.

lsmith77 commented

there are some pretty fundamental issues with this Bundle .. it sort of works for now and we will apply minor fixes if proposed. however @dantleech has already started some work on a replacement Bundle addressing the bigger issues.

dbu commented

but as we are at RC already, i expect this bundle to be part of 1.0, the new tree will come for 1.1 rather. so i would appreciate a pull request fixing this issue. i think normalizing the document_tree in the DI class sounds like the correct thing to do, rather than in javascript.

the move bug has its own issue already: #29

dbu commented

for the new bundle, we will need to decide if we keep a limiting tree configuration as here or make it totally open. imho we will have to offer both options so that people who need to be strict can be strict. in that sense such improvements will not be lost.

benglass commented

@dbu in terms of normalizing the document_tree would you mind giving me a sense of where in the code base you'd like to see that happen?

The config.nodetypes variable is populated from an instance of PhpcrOdmTree from the Sonata DoctrinePHPCRAdminBundle

Based on some searching I cannot see anywhere else in any of the cmf code that this function is used, so we could change it there. This is also where the 'undefined' is being added and I'm not sure what that is being used for so we could either remove that or the javascript would need to understand it as a special value in terms of not displaying it as a valid child when trying to Create a new document. Does the tree bundle need to handle nodes of an undefined type?

dbu commented

i think "undefined" is about the icon to use as icon for documents that have no specified icon, could that be?

for where to handle the data, i suppose the right place would be in the DependencyInjection class: https://github.com/sonata-project/SonataDoctrinePhpcrAdminBundle/blob/master/DependencyInjection/SonataDoctrinePHPCRAdminExtension.php#L106
then we can say that the tree class needs no understanding of special cases like "all" but its just a shortcut for the DI configuration that is resolved during DI handling. i don't think we need to put "undefined" into the lists having "all". but we do need to have it configured to determine how unknown document types encountered in the tree should be displayed.

dbu commented

also, in prod environment, the work of expanding the "all" will then only be done on cache:warmup instead of each request.

benglass commented

@dbu do you have any examples of the prod cache:warmup code you are referring to? I have not worked with that before.

dbu commented

that was just thinking out loud about performance implications. its part of symfony that when you run cache:warmup (or let cache:clear automatically do that) in --env=prod mode, it will dump the whole DI container to a huge funny-looking php file. but this means that things we do in the DI extension of a bundle only run once. you do not need to do something specific for that to happen, its just that the DI classes + the service configuration xml files get compiled to this php class.

benglass commented

Oh great thanks for the clarification. I will look into what the "undefined" type node is being used for and put together a PR to normalize the tree in the DI extension.

dbu commented

i think it has to do with what the jstree jquery plugin expects - try changing the image that we specify in the undefined and check if / what changes. https://github.com/symfony-cmf/TreeBrowserBundle/blob/master/Resources/public/js/jquery.jstree.js

benglass commented

I will double check but I am not seeing any indication in the jquery js tree code that "undefined" is a special value (just a lot of checks against typeof of a variable not being undefined).

It definitely has the concept of a "default" node type but it refers to it as "default" and provides a config for it which includes valid_children: "all". It seems that it is somewhat core to the jstree plugin to support a value of "all" for valid_children so it is possible that expanding that value in the DI extension may not always resolve the issue (specifically if there was ever a "default" node).

It also doesnt address the "undefined" node type which is populated with "all" for valid_children in the init.js file itself. Possibly I can just set this to $this->validClasses instead of "all". I will test but I am curious as to whether to undefined option even needs to exist if I can't find evidence of it being used by jquery.jstree (since PhpcrOdmTree.getNodeTypes doesnt seem to be used for anything except the tree and the tree doesnt seem to use undefined).

dbu commented

if you can't find anything breaking, lets simply remove the
configuration for "undefined".

dbu commented

fixed in sonata admin