Problem/Motivation

Pathauto pattern uses Condition plugins, but doesn't provide user interface for their management (except for the basic ones controlled from the pattern form).

Proposed resolution

Provide UI for condition management.

Remaining tasks

  • Move condition UI to a sub-module.
  • Write tests?
  • Document API changes? Do we need a change record?

User interface changes

  • Separate UI is added for management of additional conditions on a pattern.
  • "Conditions" operation is added to the list on the pattern listing page.
  • Local tasks are added for easier switching between the pattern edit form and condition list.

API changes

Methods added:

  • \Drupal\pathauto\PathautoPatternInterface::getLockedSelectionConditionPluginIds()

Behavior changed:

  • \Drupal\pathauto\Entity\PathautoPattern::addSelectionCondition() doesn't override UUID of the condition if it already exists.

Data model changes

No.

CommentFileSizeAuthor
#25 pathauto-pattern-condition-ui-3224916-25.patch37.16 KBmax.valetov
#24 pathauto-pattern-condition-ui-3224916-24.patch37.13 KBmax.valetov
#23 pathauto-pattern-condition-ui-3224916-23.patch37.14 KBmax.valetov
#21 pathauto-pattern-condition-ui-3224916-21.patch36.99 KBuberengineer
#20 pathauto-pattern-condition-ui-3224916-20.patch36.99 KBuberengineer
#13 pathauto-pattern-condition-ui-3224916-13.patch36.98 KBjoao.ramos.costa
#11 pathauto-pattern-condition-ui-3224916-rerolled-mr7.patch36.98 KBjoao.ramos.costa
#8 pathauto-pattern-condition-ui-3224916-8.patch44.6 KBdmitriy.trt

Issue fork pathauto-3224916

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

Dmitriy.trt created an issue. See original summary.

Dmitriy.trt opened merge request !7

dmitriy.trt’s picture

dmitriy.trt commented
Issue summary: View changes
Status: Active » Needs review

Submitted an MR. It would be great to receive some feedback before further development, e.g. what should be done in order for the changes to be accepted & merged.

dmitriy.trt’s picture

dmitriy.trt commented

Updated the MR with the following changes:

  • The form responsible for the condition type selection is now on the top of the condition list. This is better aligned with other listing UIs.
  • The add condition form displayed in a modal caused issues on different condition plugin forms, mostly on the ones that use AJAX. So, the form doesn't use modal dialog anymore.
  • Destination query parameter breaks the workflow when adding a condition. Instead of redirecting to the add condition form, Drupal redirects back to the list of patterns, so the destination has been removed.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There's at least one existing issue about this: #3077491: Add support for more advanced conditions. See my comment there as well.

dmitriy.trt’s picture

dmitriy.trt commented
Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#3077491: Add support for more advanced conditions

Thanks for feedback! Yes, moving it to a sub-module sounds possible, but my time is limited right now, so maybe later.

However, some changes have to stay in the main module (API changes described in the issue).

Also, for the entity field conditions to work properly, you may need a fix submitted to #3187430: Pathauto checks for patterns before saving field values.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, not fixed on the submodule thing, I could also think of a setting, might be easier. But I think having this extended UI as an opt-in feature is important. Otherwise setting up basic patterns that are enough for most sites involves a lot more clicks and is much harder to do.

Or a hybrid mode, keep those two conditions as they are now but allow to add more with a button below.

dmitriy.trt’s picture

dmitriy.trt commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-8.patch44.6 KB

Adding a patch file here for a stable reference from composer files. It was made from MR!7 at commit cd7daa6c.

dmitriy.trt’s picture

dmitriy.trt commented

Pushed the following updates to the MR!7:

  • Condition UI is now a separate sub-module.
  • Operation and local task are renamed a bit for consistency with other operations.

What could still be done to improve this MR and I plan to spend time on it, but not sure when:

  • The form that adds condition could also be displayed in a modal. See MR in #3230847: AJAX is broken on the add condition form for a solution that allows a condition form to use AJAX, that may be critical for some condition plugins.
  • Functional tests that check basic features of the sub-module, e.g. add/edit/delete condition, the fact that added condition actually works.
dmitriy.trt’s picture

dmitriy.trt commented

Completed the following and pushed to MR!7:

  • The form that adds condition is also displayed in a modal through a sub-request, so that condition forms could use AJAX.

It's only left to write some basic tests, at least from my point of view.

joao.ramos.costa’s picture

joao.ramos.costa commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-rerolled-mr7.patch36.98 KB

Hi @Dmitriy.trt , thanks for the work. I've already had the chance to test if functionally and it seems to me ok. Here is a patch with reroll of merge request 7 against 8.x-1.10.

joao.ramos.costa’s picture

joao.ramos.costa commented
joao.ramos.costa’s picture

joao.ramos.costa commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-13.patch36.98 KB

Hi @Dmitriy.trt , thanks for the work. It's indeed very useful.
I've already had the chance to test if functionally and it seems ok to me. Here is a rerolled patch of merge request 7 against 8.x-1.10.

(I made a little mistake in the previous patch, here it is again.)

Thank you !

dtamajon’s picture

dtamajon commented

Hi, I'm interested in that feature but trying to figure out how to add custom conditions from a custom module, as I need to add conditions based on my custom fields.

Do I have to follow some base class as in Rules module?

joao.ramos.costa’s picture

joao.ramos.costa commented

Hi @dtamajon you can create a class that extends ConditionPluginBase.
Have a look https://gist.github.com/acrosman/ab7e9ffce3996f6bc1798f9f1ca34c06. Cheers

dtamajon’s picture

dtamajon commented

Thank you!

dtamajon’s picture

dtamajon commented

I tried to add conditions successfully (custom ones and default ones) and works perfectly.

dieterholvoet’s picture

dieterholvoet commented
Issue tags: +Needs tests
dieterholvoet’s picture

dieterholvoet commented

I had to fix a couple things to make it work on Drupal 10, but apart from that this feature seems to be working! @Berdir, could you comment on whether or not this feature should live in a submodule? That way we know how to move this forward. Since this feature has lived in this MR for two years already and it seems to be working great, I'm considering creating a new project for the submodule so that people can start using it.

uberengineer’s picture

uberengineer commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-20.patch36.99 KB

Re-rolled patch 13 for Drupal 10

uberengineer’s picture

uberengineer commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-21.patch36.99 KB

Patch #20 does not apply due to whitespace #21 applies

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

max.valetov’s picture

max.valetov commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-23.patch37.14 KB

Adds a D10 session check and set for subrequest to patch 21

max.valetov’s picture

max.valetov commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-24.patch37.13 KB

Fixes patch formatting issue for patch 23

max.valetov’s picture

max.valetov commented
StatusFileSize
new pathauto-pattern-condition-ui-3224916-25.patch37.16 KB

Fix 'non-scalar value' error - patches 21-24

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

This is fantastic! Thanks for working on this. I may have time to contribute as well.

One change I think this needs is to disable the default, hard-coded conditions UI that pathauto already provides for specifying language and content type. I think that if this sub-module is used, then all conditions should be managed by it.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Please, let's keep all work in the merge request. The last three patch files are especially problematic because they don't include an interdiff.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

[double post]

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

While this is working well, it exposes a problem with the PathautoWidget. The form widget disables Pathauto if there isn't a matching pattern for the entity. But if you have only a single pattern defined and that pattern has a condition based on data from the entity, when you go to create a new Page node, the Pathauto checkbox won't be there and it won't be activated.

To resolve this, you need to create a fallback pattern that is activated for the content type without any data-based conditions. Then the widget will show the checkbox to activate pathauto.

Once the node is saved and has the required data, pathauto will re-evaluate the patterns for the node and choose the pattern that has the data condition. Though, in testing I found that you need this patch too: #3187430: Pathauto checks for patterns before saving field values

To be clear, the conditions UI that this patch provides is working as expected, it just exposes some additional challenges when using data-based conditions.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Related issues: +#3187430: Pathauto checks for patterns before saving field values
dieterholvoet’s picture

dieterholvoet commented

There seems to be an issue since the changes that were done in the past months: when updating an existing condition, it's being duplicated instead of the existing one being updated.

siliconandincense’s picture

siliconandincense commented

Hi folks!

Sorry for the beginner question but I don't have a huge amount of experience when it comes to anything more complicated than pulling in a patch.

With the release of the pre-crimbo commits do I need to manually roll a patch myself from the HEAD of this branch to get this feature D11 ready, or is there another way to do it?

Thanks for your patience!

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

You should click the "plain diff" link next to the merge request link (top of the issue on this page). It generates a git patch file. You should copy the contents of that into a plain text file in your project and include it with composer patches. Looks like it's already D11 compatible.

The thing holding up this issue is the lack of tests.

siliconandincense’s picture

siliconandincense commented

Fantastic, thanks ever so much for the reply, ill give that a go :)

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

mably’s picture

mably commented
Related issues: +#3357977: Allow strings and puncuation to be controlled by pathauto pattern
mably’s picture

mably commented
Related issues: -#3357977: Allow strings and puncuation to be controlled by pathauto pattern

mably opened merge request !153

mably changed the visibility of the branch 3224916-condition-ui to hidden.

mably’s picture

mably commented

Created a new MR from the original one to enable Tugboat.

How are we supposed to test the new UI? Not seeing anything.

EDIT: My bad, I forgot to enable the pathauto_condition_ui submodule.

dieterholvoet’s picture

dieterholvoet commented

I started a separate module project for this feature: Pathauto Conditions UI. I added @dmitriy.trt and @bkosborne as maintainers since they contributed in a significant way to the MR here.