Problem/Motivation

For #3482684: Use Drupal Conditions Plugin API to include / exclude by certain conditions we should also add some tests and test the settings form in general.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork tab_title_attention-3482770

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

anybody created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » lrwebks

Should be a quick thing, when using a good template to start. ETA: 30min

We won't test the animation itself, of course.

lrwebks’s picture

lrwebks commented
Status: Active » Needs work

lrwebks opened merge request !6

lrwebks’s picture

lrwebks commented
Status: Needs work » Needs review

Changes to the tests as well as new tests done, however phpunit does not seem to be overly happy about the schema for the conditions.

I see two options here:

1. Define the schema differently (I am not that familiar with the dynamic schema for conditions though)
2. Use the actual settings page to change the settings in the tests (Not a friend of that since it should also work when changed via configFactory)

Any suggestions on how to proceed?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Our current implementation is very similar to what block.schema.yml implements:

    visibility:
      type: sequence
      label: 'Visibility Conditions'
      sequence:
        type: condition.plugin.[id]
        label: 'Visibility Condition'

Here's an example from /core/modules/block_content/tests/modules/block_content_test/config/install/block.block.foobar_gorilla.yml:

visibility:
  request_path:
    id: request_path
    negate: false
    pages: ''

So I can't see major differences and think the mistake might not be on our side, but on the side of the condition plugin that causes this issue?

Wondering why it doesn't fail in core block tests then? Any ideas?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

I compared everything to web/core/modules/system/tests/modules/condition_test/ and it looks fine, so I think this is test-specific.

Looking at the condition tests, I saw they were using the "system" module! Perhaps that's what we need.
If it's not enough, we could check if condition_test test module dependency is required, but I don't think so.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Yeah it's definitely from the "system" module and it defines the schema correctly:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/config/schema...

negate etc. are inherited!

It totally makes sense to require the "system" module though, as it's where request_path Condition Plugin is implemented:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...

But no more idea, why it still doesn't fix the failing tests.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: Write tests for the settings form and Conditions » Write tests for the settings form and Conditions & fix schema
Status: Needs review » Reviewed & tested by the community

Okay I'm going to merge the tests now, as they work as expected. No more idea what's causing the schema to fail, but let's solve this one day...

  • anybody committed 7fbaf015 on 1.x authored by lrwebks 
    Issue #3482770 by anybody, lrwebks: Write tests for the settings form...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Needs work

Back to NW for the schema issue.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: lrwebks » Unassigned
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Okay sadly lots of time wasted. If we just had a look at the line, where the Schema error is reported, we'd have seen that this is NOT a schema error, but a bug in the tests implementation...

On it!

anybody opened merge request !7

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs work » Fixed

Okay all green now. I believe the other tests fail because of #2783897: Current theme condition plugin should provide an option to match all themes and introduce checkboxes

Follow-Up: https://www.drupal.org/project/tab_title_attention/issues/3485667

  • anybody committed 43a3c898 on 1.x 
    Issue #3482770 by anybody, lrwebks: Write tests for the settings form...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.