New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong result in NodeType->isOfType() if a superType of a Mixin is disabled #1983

Closed
sbruggmann opened this Issue Apr 3, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@sbruggmann
Copy link
Contributor

sbruggmann commented Apr 3, 2018

The FlowQuery q(node).is('[instanceof Vendor.Package:DisabledInheritedSuperType]') returns true instead of false.

Steps to Reproduce

Given we have a "Person"-NodeType which is only accessible in the backend:

'Vendor.Persons:Person':
  superTypes:
    'Vendor.Site:Document': true
    'Vendor.Site:FrontendRedirectToParentPage': true
  ...

And than we use the "Person" NodeType as superType for a new NodeType "Speaker" which has to be accessible in the Frontend too:

'Vendor.Conference:Speaker':
  superTypes:
    'Vendor.Persons:Person': true
    'Vendor.Site:FrontendRedirectToParentPage': false

Actual and expected behavior

This works in the Neos Backend, inspector, etc.

But if we check for it with EEL/FlowQuery q(node).is('[instanceof Vendor.Site:FrontendRedirectToParentPage]') it returns for both NodeTypes true, but the second should return false.

Identified problem

The method NodeType->isOfType() doesn't know that there should be a superType disabled, because NodeType->declaredSuperTypes don't list which should be disabled but also don't remove the overwritten ones.

Affected Versions

At least Neos 3.3

@sbruggmann

This comment has been minimized.

Copy link
Contributor

sbruggmann commented Apr 3, 2018

I could fix my specific problem by replacing

foreach ($this->declaredSuperTypes as $superType) {
    if ($superType !== null && $superType->isOfType($nodeType) === true) {
        return true;
    }
}

in \Neos\ContentRepository\Domain\Model\NodeType->isOfType
with

if (array_key_exists('superTypes', $this->getFullConfiguration())) {
    foreach ($this->getFullConfiguration()['superTypes'] as $superTypeName => $superTypeEnabled) {
        if ($superTypeEnabled !== false && $superTypeName == $nodeType) {
            return true;
        }
    }
}

But I don't see the whole picture and I think that this maybe has to be fixed on the declaredSuperTypes level.

@bwaidelich bwaidelich self-assigned this Jun 18, 2018

@bwaidelich bwaidelich removed their assignment Jul 10, 2018

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Jul 10, 2018

Unassigned myself because I didn't manage to tackle this one before my holidays :-/
But it's on my radar!

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Aug 8, 2018

@sbruggmann finally.. :) your implementation was a bit "expensive" and had some unwanted side effects, but this should hopefully do the trick: #2139

kitsunet added a commit that referenced this issue Aug 8, 2018

Merge pull request #2139 from bwaidelich/1983-bugfix-isOfType-respect…
…s-disabled-superTypes

BUGFIX: Respect disabled superTypes when filtering by Node Type

This fixes `NodeType::isOfType()` to return `false` when the given
type is explicitly disabled by the corresponding NodeType or one of
it's super types.

Example:

    'Acme:Animal':
      superTypes:
        'Acme:HasBonesMixin': true

    'Acme:Jellyfish':
      superTypes:
        'Acme:Animal': true
        'Acme:HasBonesMixin': false

With this `NodeTypeManager::getNodeType('Acme.Jellyfish')->isOfType('Acme:HasBonesMixin')`
should return `false` but it didn't.
Respectively a FlowQuery like the following should not return any "Jellyfish"-node (or
nodes with a sub-type): `q(node).find('[instanceof Acme:HasBonesMixin]')` but it did.

Fixes: #1983

bwaidelich added a commit to bwaidelich/neos-development-collection that referenced this issue Aug 10, 2018

!!! BUGFIX: Respect disabled superTypes when filtering by Node Type
This fixes `NodeType::isOfType()` to return `false` when the given
type is explicitly disabled by the corresponding NodeType or one of
it's super types.

Example:

    'Acme:Animal':
      superTypes:
        'Acme:HasBonesMixin': true

    'Acme:Jellyfish':
      superTypes:
        'Acme:Animal': true
        'Acme:HasBonesMixin': false

With this `NodeTypeManager::getNodeType('Acme.Jellyfish')->isOfType('Acme:HasBonesMixin')`
should return `false` but it didn't.
Respectively a FlowQuery like the following should not return any "Jellyfish"-node (or
nodes with a sub-type): `q(node).find('[instanceof Acme:HasBonesMixin]')` but it did.

This might be a breaking change if Fusion or PHP code relies on the broken behavior.
NodeTypes with disabled `superTypes` should be tested for this.

Fixes: neos#1983

bwaidelich added a commit that referenced this issue Sep 5, 2018

!!! BUGFIX: Respect disabled superTypes when filtering by Node Type (#…
…2147)

This fixes `NodeType::isOfType()` to return `false` when the given
type is explicitly disabled by the corresponding NodeType or one of
it's super types.

Example:

    'Acme:Animal':
      superTypes:
        'Acme:HasBonesMixin': true

    'Acme:Jellyfish':
      superTypes:
        'Acme:Animal': true
        'Acme:HasBonesMixin': false

With this `NodeTypeManager::getNodeType('Acme.Jellyfish')->isOfType('Acme:HasBonesMixin')`
should return `false` but it didn't.
Respectively a FlowQuery like the following should not return any "Jellyfish"-node (or
nodes with a sub-type): `q(node).find('[instanceof Acme:HasBonesMixin]')` but it did.

This might be a breaking change if Fusion or PHP code relies on the broken behavior.
NodeTypes with disabled `superTypes` should be tested for this.

Fixes: #1983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment