Problem/Motivation

The context (entity_bundle:node) is required. This should not be the case.

Steps to reproduce

Select a content type (bundle) (for example "Article").
Negate the condition which means the condition has to be true except the content type is equals "Article".
The problem then is if there is an entity which has not a node/content type (for example a view from the Views module) the plugin evaluate to false although the condition is given that the (not existing) node is not of content type "Article" but because the context is missing it ends in false.

Proposed resolution

- Make entity bundle context optional - setRequired to FALSE
- Make sure the entity is instanceof ContentEntityInterface for EntityBundle Plugin Condition evaluate method.
- Update tests - EntityBundleConditionTest - accordingly ... with the right context

Remaining tasks

CommentFileSizeAuthor
#63 2823432-nr-bot_bjfhkds8.txt548 bytesneeds-review-queue-bot
#51 2535896-51.patch3.78 KBtimohuisman
#42 2823432-nr-bot.txt90 bytesneeds-review-queue-bot
#24 interdiff_19-24.txt577 bytesmeenakshi_j
#24 2823432-24.patch1.15 KBmeenakshi_j
#19 2823432-19.patch1.18 KBhardik_patel_12
#18 2823432-18.patch1.15 KBhardik_patel_12
#10 2823432-fix_evalution_of_node_type_condition_plugin.patch1.12 KBantonnavi
fix_evalution_of_node_type_condition_plugin.patch2.02 KBTiMESPLiNTER

Issue fork drupal-2823432

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

TiMESPLiNTER created an issue. See original summary.

chi’s picture

chi commented
Title: NodeType condition plugin evaluation is sometimes wrong » Node Type condition evaluation is wrong when context is not provided
Version: 8.2.1 » 8.3.x-dev
Assigned: TiMESPLiNTER » Unassigned
Status: Active » Needs review
chi’s picture

chi commented
Version: 8.3.x-dev » 8.2.x-dev
TiMESPLiNTER’s picture

TiMESPLiNTER commented

Any news about this issue? What work is required to bring this fix into the next release?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

This patch doesn't seem to have any tests. Let's start by adding those.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

antonnavi’s picture

antonnavi
Primary language Ukrainian
Location Leipzig
commented
Status: Needs work » Needs review
StatusFileSize
new 2823432-fix_evalution_of_node_type_condition_plugin.patch1.12 KB

Hi all here!
I got the same issue on my project, but this patch didn't help me. I modified it to cover my issue case too.

Status: Needs review » Needs work

The last submitted patch, 10: 2823432-fix_evalution_of_node_type_condition_plugin.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#1932810: Add entity bundles condition plugin for entities with bundles
antonnavi’s picture

antonnavi
Primary language Ukrainian
Location Leipzig
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2823432-fix_evalution_of_node_type_condition_plugin.patch, failed testing. View results

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.8.x-dev » 8.9.x-dev

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review
StatusFileSize
new 2823432-18.patch1.15 KB

Re-rolled against 9.1.x-dev.

hardik_patel_12’s picture

hardik_patel_12 commented
StatusFileSize
new 2823432-19.patch1.18 KB

Kindly ignore previous patch forgot to remove "node" = @ContextDefinition("entity:node", label = @Translation("Node"))

Status: Needs review » Needs work

The last submitted patch, 19: 2823432-19.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Related issue committed

meenakshi_j’s picture

meenakshi_j commented
Status: Needs work » Needs review
StatusFileSize
new 2823432-24.patch1.15 KB
new interdiff_19-24.txt577 bytes

Fixed the fails #19

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

What #23 was trying to say is that this now also needs to update the new EntityBundle condition accordingly.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Related issues: +#2887071: Page title block settings removes pages titles from views pages and group pages

Related: #2887071

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vasike made their first commit to this issue’s fork.

vasike opened merge request !2879

vasike’s picture

vasike
Location Ramnicu Valcea
commented
Title: Node Type condition evaluation is wrong when context is not provided » Node Type / Entity bundle conditions evaluation is wrong when context is not provided
Component: node system » entity system
Status: Needs work » Needs review
Related issues: +#2535896: ConditionManager::evaluate() should not negate results itself

There is a new MR for D10, but only for Entity bundle condition, as "Node Type" is history.

However this will not solve "all problems" with the condition that have negate option set to TRUE.

For this i think we need to link this with https://www.drupal.org/project/drupal/issues/2535896

And of course any suggestions for tests that are required ... could help.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs issue summary update

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Moving back to NW for the tests.

Hiding the patches as the MR seems to be the approach going forward. Appears to be failures in the MR.

Tagging for IS update to follow the standard template.

vasike’s picture

vasike
Location Ramnicu Valcea
commented
Status: Needs work » Needs review

- Rebase the MR
- Update - Small test for missing context for EntityBundle condition - without this update will fail.

vasike’s picture

vasike
Location Ramnicu Valcea
commented

It seems the latest updates revealed issue with testing the block with node bundle condition - missing route context ...
Now it should be "completed".

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests

Seems tests have been added but IS update is still needed

Did not test yet.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vasike’s picture

vasike
Location Ramnicu Valcea
commented
Version: 11.x-dev » 10.1.x-dev
Issue summary: View changes
Status: Needs work » Needs review

updatet summary and MR rebased.

smustgrave’s picture

smustgrave commented
Version: 10.1.x-dev » 11.x-dev

11.x is the latest development branch. MR should probably be updated to point to that.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Ran the tests locally to make sure they failed without the fix

Behat\Mink\Exception\ElementNotFoundException : Form field with id|name|label|value "visibility[entity_bundle:node][context_mapping][node]" not found.

and

Drupal\Component\Plugin\Exception\ContextException : The 'entity:entity_test_with_bundle' context is required and not present.

Which is good

Thanks for updating the issue summary, removing that tag

Think this 7 year issue is ready for committer review.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs review

Doing RTBC triage.

The issue summary is up to date and I see that a remaining task is 'code review'.

The issue title was changed on GitLab but that is not reflected here. Can someone fix that?

Reading through the comments there is work on the patch, addition of tests and rerolls but there is no code review that I can find. I do see that @smustgrave ran a fail test locally, that is good to know. But still in need of eyes on the code.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2823432-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

vasike opened merge request !4610

vasike’s picture

vasike
Location Ramnicu Valcea
commented
Status: Needs work » Needs review

new MR for 11.x available and passed, so, i think we're back to Needs review

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

All green for 11.x so think this is good to go back.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

There are two open merge requests in the issue summary. Please close the non-canonical one, or if you don't have permission to close it, document which should be closed in an issue comment and the IS so a committer can close it for you. Thanks!

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Updated proposed section

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Thanks @smustgrave. I think maybe we should put in at least an h4 for this, or possibly add it under a separate h3, so that it's obvious on scanning. I updated the summary accordingly.

xjm closed merge request !2879

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review, +Needs change record, +Needs tests, +Needs manual testing

Posted some comments on the MR. I'd also like to see some manual testing documented for node and non-node entity bundles. Thanks!

timohuisman’s picture

timohuisman
Primary language Dutch
Location Leiden, Netherlands
commented
StatusFileSize
new 2535896-51.patch3.78 KB

Attached is a snapshot of the current state of MR 4610, so it can be safely used with composer-patches.

I came across this issue through #3264843: Add support for negating node type(content type) condition for block visibility, which seems to work in combination with MR 4610.

However, I've tried to test MR 4610 standalone, but I'm not sure what it should resolve. If I understand this issue correctly the MR should resolve the fact that blocks are hidden on non-node-type routes when there is a node bundle condition configured. I've add a condition for "Article" nodes to the page title block, visited a non existent page so I could see the 404 page but the page title was still hidden.

oily changed the visibility of the branch 11.x to hidden.

oily’s picture

oily commented
Issue summary: View changes
oily’s picture

oily commented
Issue summary: View changes
oily’s picture

oily commented

Edited the issue summary. Simplified the description and steps to reproduce and remove duplicate sections.

Okay, just read all comments and realised the issue summary has been edited already. Please revert my changes if they aren't useful.

oily’s picture

oily commented

Rebased.

There are still code comments to be resolved in the MR especially in the tests. The issue tags show other tasks to be completed.

oily’s picture

oily commented
vasike’s picture

vasike
Location Ramnicu Valcea
commented
Status: Needs work » Needs review

MR updated with latest 11.x + some feedback for MR review.

Let's try again a review. thanks

xjm’s picture

xjm
she/her
Primary language English
commented

Amending attribution.

smustgrave’s picture

smustgrave commented
Issue tags: -Needs subsystem maintainer review +Needs framework manager review

Been over 4 months and no sub-maintainer has freed up so lets bump to framework managers.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

acbramley’s picture

acbramley commented

Saw this pop up in the needs review slack - my first thoughts are that this may cause pretty major issues for existing sites (e.g a bunch of blocks may start displaying where they didn't before).

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2823432-nr-bot_bjfhkds8.txt548 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.