verbb / navigation

A Craft CMS plugin to create navigation menus for your site.
Other
90 stars 23 forks source link

Custom NodeType breaks #162

Closed loweoj closed 4 years ago

loweoj commented 4 years ago

Description

I'm writing a custom node type to populate url from a global set. My intention is to replace {{ globalSet.field }} in the url field with the contents of the set, if it exists. I'm doing this in the getUrl()function on the custom NodeType and I'm running into a few issues with this:

1) NodeType::getUrl() does not have access to $this->node when saving a new node. It looks like this is because $node never gets set on the NodeType in NodesController::actionSaveNode(). Maybe $node could get set when nodeType is accessed within Node::getUrl() at this point:

if ($this->nodeType()) {
    return $this->nodeType()->getUrl();
}

2) NodeRecord::url field never gets saved in navigation_nodes. I think this is because in Node::afterSave $record->url get's set to null if $this->type is truthy. Makes sense for "Entries", but not for all custom types:

// Don't store the URL if its an element. We should rely on its element URL.
if ($this->type) {
    $record->url = null;
}

3) On MultiSites - When I create a new SiteType, then open the newly created node in the pop-up editor, the "Type" has been incorrectly set to my custom NodeType, and not "SiteType" as expected. The culprit here is the this line of javascript: var type = this.$nodeTypeForm.parents('[data-node-type]').data('node-type'); Which returns all parents where [data-node-type] is set. It so happens that in my case, the type selected is from my custom NodeType and not the expected SiteType. I hope that makes sense. Reproduction instructions below.

Steps to reproduce

  1. Create a custom node type and register it in a module/plugin:
    
    <?php

namespace modules\company\navigation;

use \Craft; use verbb\navigation\base\NodeType;

class GlobalSet extends NodeType { // Static // =========================================================================

public static function displayName(): string
{
    return 'Globals';
}

public static function hasTitle(): bool
{
    return true;
}

public static function hasUrl(): bool
{
    return true;
}

public static function hasNewWindow(): bool
{
    return true;
}

public static function hasClasses(): bool
{
    return true;
}

public function getUrl($includeSuffix = true)
{
    // check each reproduction issue for the return value here
}

}



1.
    a. Add `return $this->node->getRawUrl()` to the new GlobalSet::getUrl() 
    c. Add a CustomType node in the control panel. Notice error thrown $this->node is not available.

2. 
    b. Add `return "static url"` to the new GlobalSet::getUrl() 
    c. Create a new "Globals" node in the control panel with url "RAW URL".
    d. Notice that `navigation_nodes` table does not contain expected "RAW URL" in the `url` field of the newly created node.

3. 
    a. Add an additional Site to craft to enable "multi-site" mode.
    b. Create a new "Sites" node, enter any title and select any site.
    d Click on the newly created node in the node list and click "gear icon" to view the pop-up editor.
    e. Notice that the "Type" has been incorrectly set to "Globals".

### Additional info

- Plugin version: 1.3.19
- Craft version: 3.4.24
engram-design commented 4 years ago

All items addressed in 1.3.20, thanks for the feedback!

loweoj commented 4 years ago

Thanks so much for the fast fixes!