FEATURE: Skip specific node type or namespace from indexing

[dfeyer]

No description provided.

[kdambekalns]

Looks good, nice one!

[daniellienert]

Hey @dfeyer,
nice feature, but I am not convinced of how it is configured.
NodeType specific configuration for the indexing is done in the nodeType configuration, I think the "excludeFromConfiguration" flag should be in the nodeType configuration too and not in a separate configuration. If we have it in nodeTypes, we get the superType resolving for free.

Is there a reason why you configure this separately?

[daniellienert]

I wonder why the exclude setting is not part of the NodeType configuration. See my other comment for details.

[kdambekalns]

Indeed… good point! Also, if in the settings, it should probably be below Neos.ContentRepository.Search, since that functionality is not only related to Elasticsearch, but could/should be supported elsewhere as well.

[dfeyer]

Make sense to move it the the NodeType configuration, I can do that next week.

[dfeyer]

@daniellienert But Full namespace notation (Neos.Neos:*) will not be supported and its quiet nice to have, so I think decoupling this configuration from the NodeType make sense, @kdambekalns what do you think ?

Fine to move to Neos.ContentRepository.Search, but keep this configuration in the Settings

[dfeyer]

So now the Settings are moved to ElasticSearch.ContentRepositoryAdaptor.Search.defaultConfigurationPerNodeType

So I think this can be merged, I will push a PR to the ElasticSearch.ContentRepositoryAdaptor.Search tomorrow to add default configuration and update the README there and maybe ping @kitsunet to adapt the Sqlite Adapter too.

[daniellienert]

@dfeyer: Sorry, for me it still feels wrong to have this type of configuration somewhere else as in the nodetype configuration and to use a “.*” notation instead of the NodeType inheritence. It is not consistent with the other configuration options we have.

Maybe we find a solution for the use case to exclude a complete Package.

But I don't want to block it just for my feeling. Would be great to get some more opinions on that. Maybe from @kitsunet or @skurfuerst. If really think its a good idea, it's fine to dismiss my review.

[dfeyer]

Maybe we find a solution for the use case to exclude a complete Package.

Basically a node type namespace is not a package name, so no way to have something to exclude a package, here I want to exclude a NodeType namespace ;) same name than a package key, but different concept.

to use a “.*” notation instead of the NodeType inheritence

And nothing to do with Node type inheritance all the node types of a given namespace doest not inherit from a single parent node they are just part of the same namepace, check the Neos.NodeTypes package, you can not use inheritance for this case. So if someone add a new node type you need to exclude it explicitly (certainly after something break, if you are not aware of the addition)

Overview of the configuration is also more easy with this solution, you have a single place to check.

And as said earlier, I'm fine to have a configuration per node type, but basically the priority check must be (if this PR is accepted) Settings then NodeType options.

The configuration can be extended to include child nodes in a future iteration:

Neos:
  ContentRepository:
    Search:
      defaultConfigurationPerNodeType:
        'Neos.Neos:Shortcut':
          indexed: false
          includeChildNodes: true

All arguments exchanged

[dfeyer]

During the sprint I will update the PR to cleanup the settings and prepare a PR for the Search package, to make this feature visible. And ping @kitsunet if he's ok to implement it in the SqlLite adaptor.

That review was a bit old, wasn't it?

[kdambekalns]

Looks good and works in practice with unchanged settings.