🇨🇦Canada @tame4tex

Not sure on the best path forward here but it highlights the need for more comprehensive testing of different data types. Happy to help with this once there is a consensus on direction.

Just as a head up...

Given what I have found out in 🐛 Encrypted decimal field value not saved on drupal version 10.2.x+ Active where NULL is already inadvertently being returned by FieldEncryptProcessEntities::getUnencryptedPlaceholderValue for a decimal field, I am guessing changes would also need to be made to ProcessEntities::setEncryptedFieldValues for this not to be problematic.

Also based on 🐛 ProcessEntitiesTest is allowing unexpected results to slip through Needs work more testing beyond ProcessEntitiesTest is definitely need.

That said, I agree with the motivation behind this change.

I have created MR !73 to add both testing and a fix for this bug.

Modifying ProcessEntitiesTest was not enough to test for this bug as it seems this unit test is allowing unexpected results to slip through. I have added a separate issue for this 🐛 ProcessEntitiesTest is allowing unexpected results to slip through Needs work .

I therefore added Functional testing to ensure there is adequate coverage for this bug.

tame4tex → created an issue.

tame4tex → created an issue.

I have added a new MR !72 for the 3.2.x branch which includes tests for the bug.

Let me know if there is anything else needed to get this committed. Thanks!

Updated the MR to add a a pageshow event listener which will prevent the bug from occurring in the first place. As suggested on https://web.dev/articles/bfcache.

I still think it is worth the extra measure to keep the validation and sanity check in place "just in case".

I have added MR !91. This was inspired by on_configure_form_every_entity_is_selected-3199534-02.patch and although it doesn't specifically fix the bug, it prevents the bug from causing unexpected data changes by throwing a validation error when the bug conditions occur.

The problem with patch #2, apart from needing to be modified to apply to updated code, is it just displays the error message No items selected.. After getting the error, if the user continuously selects options without reloading the page, the same error message will keep appearing. So I updated the error message to provide a link to reload the page , along with helpful info to prevent the problem form occurring in the first place.

Ideally the code needs to be modified to correctly handle back/forward cache, but I believe that is best served in another issue as it is more complex and will most likely need greater input from @graber.

So it took some fiddling with the chrome options to get back/forward cache working but the test is now failing as expected.

https://git.drupalcode.org/issue/views_bulk_operations-3199534/-/pipelin...

I will take a look at the patch from #2 tomorrow and create a MR with a potential fix.

I have been able to replicate this bug on version 4.3.1.

In our instance, we had custom functionality that added a hidden element to the Views Exposed Form that was unique for each new form build.

Here is the workflow of the cause of the bug:

1. When the VBO form was submitted again after returning via the browser back button, the exposed form input had changed, triggering $this->tempStoreData['list'] to be cleared in ViewsBulkOperationsBulkForm::updateTempstoreData().
2. Then in ViewsBulkOperationsBulkForm::viewsFormSubmit() because the selected key was not in $this->tempStoreData['bulk_form_keys'] nothing got re-added to $this->tempStoreData['list'].
3. Then finally in ViewsBulkOperationsActionProcessor::executeProcessing() because $data['list'] is empty it gets set to all view results and ALL view results get processed!!

If we removed the custom hidden element, the bug didn't occur.

Even though it is a specific set of circumstances that would trigger the bug, I feel it is a critical issue given that data loss (e.g. with delete node action) or security issues (e.g. with unblock users action) could occur.

I have been able to create a FunctionalJavascript test locally but it requires the browser to be in non headless mode. Not sure if it will work with Gitlab CI but I will add a MR for this test shortly.

@andre.bonon are you able to provide code example of your problem as I am trying to ascertain how it is different to the current test.

Unfortunately Patch #2 is not the correct solution due to 🐛 Form API #states property/states should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations) Needs work and as nod_ has explained in previous comments. Therefore we need to work on an alternate solution and getting more info on your problem would be extremely helpful.

@mdsohaib4242 was your comment for @andre.bonon or does it relate to the current MR in some way? Thanks!

Updated title to also include Result Summary @total token.

This has also broken any site that used @current_record_count of @total in a Results Summary when using a "Some" pager.

I have opened 🌱 "Some" Pager Plugin and [view:total-rows] Regression Issue Active to address this.

I have updated the Issue summary with the additional regression issue and the two possible resolutions I have thought of so far.

I have also added a MR for Option #1 which is now my preferred.

BC is definitely a consideration but I would consider it less so compared to the regression issues resulting from the change.

It would only impact sites that started using the [view:total-rows] token from version 10.1.0 because prior to that version it returned the total number of items before the pager limit was applied.

Similar to the token issue, we have a site where the usage of Displaying @current_record_count of @total on a block using the "Some" pager is now broken due to this change.

This is what it looked like from D8 all the way to D10.1.0

This is how it looks now

On further thought, on reading the issue summary of 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Fixed again, using @current_record_count in their header rather than [view:total-rows] would have solved their problem and there would have been no need for the code change.

So on further thought maybe the changes in 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Fixed should be rolled back and we instead update the description of [view:total-rows] token to explain it is the total number of items before any limiting by the page is applied.

Right now I am just proposing options, hoping to get feedback on preferred approach.

For those interested, I have added an issue and MR to re-add the functionality that switches a select widget to an autocomplete widget when the number of options hits a predetermined limit.

See ✨ Views EntityReference filter: Add functionality to switch from select to autocomplete widget when a max number of options reached Active

I have added MR 9866 to add this feature. Code was inspired by functionality that existed in https://www.drupal.org/files/issues/2020-09-30/2429699-351--on-9_1_x.patch → on 📌 Add Views EntityReference filter to be available for all entity reference fields Closed: duplicate .

Another reminder that you MUST follow the instruction on ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Needs work under the "How to use" heading to set up the EntityReference filter first for this to work.

tame4tex → created an issue.

Actually NVM, sorry my bad, I see where it is being applied.

This is not an issue with this code, it is a method further along the call tree in \Drupal\views\Plugin\EntityReferenceSelection\ViewsSelection::getReferenceableEntities that is not respecting the limit. I will investigate further and open a separate issue if need be.

Switching back to Reviewed & tested by the community. Sorry for the noise!

I can't see where in the code the Select list maximum number of options of 100 gets applied. I just did a manual test with 103 options and they all appeared in the select list.

Is this an oversight? Should the number of options be limited to 100?

Switching back to Needs Work (Sorry!)

Given there wasn't a lot of work involved, and to hopefully speed things along, I have created a MR with the proposed resolution.

Even though this should fix the related issues, it doesn't automatically close them as testing should be added for the specific bug experienced, ie visibility of the More link and how caching effects this visibility.

I feel the solution pursued so far may possibly be the wrong approach.

It was the solution to 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Fixed that triggered this regression. It was fixing the value returned for the token [view:total-rows] that resulted in PagerPluginBase::$total_items always being set to the number of limited query results for the "Some" pager.

But what if this assumption was incorrect, what if [view:total-rows] should not always return number of limited query results. For example, if I am using the "Some" pager, limited to 5 results when there are 20 and I have a "More link" enabled, I think it would be valid for me to want to add the text "Displaying 5 or 20 results". In which case I would want [view:total-rows] to return 20.

If the code is changed to only override PagerPluginBase::$total_items in the postExecute() if a "More link" is not enabled, then this issue would be fixed.

I have opened another issue ( 🌱 "Some" Pager Plugin and [view:total-rows] Regression Issue Active ) to discuss this proposal as it may require other actions unrelated to this issue.

tame4tex → created an issue.

So I finally had some time to revisit this issue and add states testing.

I have added the states testing in a separate issue https://www.drupal.org/project/time_field/issues/3479769 📌 Add form conditional states testing Active to keep this issue as simple as possible.

It turns out my initial assumption was incorrect, states is actually working. It was our custom code that interacts with states that was failing due to expecting an empty string would mean empty.

Regardless, I am still going to work on the proposed resolution to see if NULL will work. In my opinion it is a much better approach than relying on 86401, if for no other reason than to get rid of the console warnings.

I have added the testing and opened the merge request.

Contrary to my initial thoughts in https://www.drupal.org/project/time_field/issues/3463912 🐛 The integer 86401 should not be used to represent an "empty" time element Active , javascript states is actually working.

Ready for review.

tame4tex → created an issue.

This is no longer an issue in 3.2.x so it can be closed.

Closing this as a duplicate to https://www.drupal.org/project/drupal/issues/3468860 🐛 JS #states behavior does not have a detach method Needs review where there is a proposed fix to address ajax related states issues.

Please review the MR on that issue to see if fixes your problem.

So this fix has an unexpected affect in that the revision log message textfield is no longer displayed on node create forms.

I think what is happening with the fix is the states added to the revision_log_message in \Drupal\Core\Entity\ContentEntityForm::addRevisionableFormFields() is now being correctly handled because the revision checkbox is not present in the DOM so the field shouldn't be visible.

I wonder if it was intended to be shown or if this is a bug that slipped through. I found this possible related issue https://www.drupal.org/project/drupal/issues/3096906 ✨ Don't show the revision log message on creation of content, media, etc. Needs work . Either way it would need to be addressed before this fixed is merged in.

Would be great to get a maintainers input on this. Thanks!

All failing tests are fixed. Good for review.

I found another bug.

If the ajax added dependee includes the form in its selector it doesn't work. It is completely valid for the dependee selector to include a form identifier if there are multiple forms on a page that have the same fields.

This bug is caused by using the find() method on the context when checking if the dependee is now present in the dom. This won't work when the selector contains form because on ajax reload the form will be the context root .

We need to instead use the jQuery Selector $(), which seems valid given that is what is used to apply the event listeners and also what was used to determine if the element is in the dom in the first place.

I have added testing for this bug and fixed it in the latest MR commits. But it looks like the changes has caused other failing tests. I'm investigating now.

I have updated the MR and added to the great start by sukr_s. I was going to make code suggestions to the MR but there were a lot of changes so decided to commit to expedite things. I hope you don't mind.

All tests are now passing and its ready for review.

Summary of changes made to the MR:

1. Renamed the statesObjects property to processedDependees to hopefully be more descriptive of what it is.
2. Modified the states.Dependent function to keep track of any dependees not in the DOM so they can be rechecked on attach. Also ensured Dependents are re-evaluated if none of their dependees are in the DOM to ensure states are reprocessed when dependees are removed via ajax.
3. Fixed issue in Drupal.states.Dependent.destroy where it was removing the state:${dependeeStates[i]} event associated with other Dependents
4. Fixed issue where duplicate entires were being added to statesObjects property when states were reprocessed on ajax
5. Fixed issue in Drupal.states.Trigger.destroy where trigger related event listeners were not being removed.
6. Added functionality to the state of the attach function to ensure states are reprocessed for dependee elements that are added to the DOM during ajax

I have updated the MR and added more comprehensive testing to highlight the ajax related bugs that still exist.

I moved the testing added to its own new method.

These new tests should cause the MR to fail as expected.

I have a working solution to fix all remaining issues that I am just doing final cleanup on and will commit and comment on shortly.

Thank you for your efforts so far but this doesn't completely fix the issue. I still have an issue with an element that is added by ajax and is the states dependee of an element that is not affected by ajax.

Here is a modified version of the form in the issue description that will highlight the bug.

Steps to reproduce

1. Create with form with Ajax and states as below.

    $form['js_states_test'] = [
      '#type' => 'details',
      '#open' => TRUE,
      '#prefix' => '<div id="js_states_test_wrapper">',
      '#suffix' => '</div>',
    ];
    $form['js_states_test']['reload'] = [
      '#type' => 'checkbox',
      '#title' => 'Check me to reload Select Field field',
      '#ajax' => [
        'callback' => '::buildAjax',
        'wrapper' => 'js_states_test_wrapper',
      ],
    ];
    $form['js_states_test']['select_field'] = [
      '#type' => 'select',
      '#title' => 'Select Field',
      '#options' => [0 => 0, 1 => 1],
      '#default_value' => 0,
    ];
    if ($form_state->getValue('reload')) {
      $form['js_states_test']['dependent_checkbox'] = [
        '#type' => 'checkbox',
        '#title' => 'Only visible if reload is checked',
      ];
    }
    $form['js_select_field_textfield'] = [
      '#type' => 'select',
      '#title' => 'Select should show when 1 is selected in select_field after ajax',
      '#states' => [
        'visible' => [
          ':input[name="select_field"]' => ['value' => 1],
        ],
      ],
    ];
    $form['js_checkbox_field_textfield'] = [
      '#type' => 'textfield',
      '#title' => 'Field should hide when dependent_checkbox is checked after ajax',
      '#states' => [
        'invisible' => [
          ':input[name="dependent_checkbox"]' => ['checked' => TRUE],
        ],
      ],
    ];

2. Click on "Check me to reload Select Field field" checkbox
3. Select value 1 for "Select field"
4. BUG: 'Select should show when 1 is selected in select_field after ajax' is not visible when it should be
5. Click on "Only visible if reload is checked" checkbox
6. BUG: "Field should hide when dependent_checkbox is checked after ajax" is not hidden when it should be

NOTE: This is a very simplified version of the problem we are experiencing. I know the "dependent_checkbox" doesn't have to be added via ajax and can easily be made visible via states based on "reload" and then there is no bug, but that is not possible in our situation. This is just an easy way to demonstrate the bug.

In addition to the current MR !9300 changes which fixes #4 above, I suggest removing the dupal-once attribute if one or more of the dependees are not present in the DOM. This will ensure the states of that element get processed again when the DOM is updated.

Thoughts?

I will add suggested code as comments to the MR !9300.

Sure we could list the file create time but that is overkill for our current usage. It is simply a "nice to have". I guess we got used to the original naming convention and found it helpful at times.

It doesn't seem like I am going to win you over, no worries, thanks for considering.

Ugh ... sorry. Never mind this was already fixed in https://www.drupal.org/project/signaturefield/issues/3422334 🐛 Library definition license is missing URL Fixed .

Any chance of getting an updated release soon? Thanks!

tame4tex → created an issue.

In our usage it provides some helpful context as to when the signature was created by just looking at the filename. We don't have to then go and check the file create date.

What are the benefits of keeping the current approach?

Assuming this wont be addressed in core, I have added a MR to fix the extra empty column issue.

I have refactored paragraphs_preprocess_field_multiple_value_form quite a bit in an attempt to improve readability and improve robustness.

I have manually tested on both paragraph widget types and all looks to be ok.

Added related issue

As @nsavitsky confirmed this is not completely fixed.

template_preprocess_field_multiple_value_form() is automatically adding an extra table column for a form element item's '_actions'. \Drupal\Core\Field\WidgetBase::formMultipleElements() adds the '_actions' array key and populates it with the item's remove button.

Given \Drupal\paragraphs\Plugin\Field\FieldWidget\ParagraphsWidget::formMultipleElements() does not add '_actions' to its items, we end up with this extra empty column.

The question is should this be fixed in core or paragraphs? Should core automatically add a column for '_actions' without confirming that items do actually have that key?

I believe this has been fixed by Stop using $error->arrayPropertyPath (3333974) → and is in release 1.18. Thus I am closing as duplicate.

tame4tex → created an issue.

I have created MR with the requested change

tame4tex → created an issue.

Added a comment to the MR as I don't think we should be adding an empty page parameter if we don't need to.

tame4tex → created an issue.

Attached patch from MR for those that prefer a patch file.

tame4tex → created an issue.

Could the issue be more easily solved in JS by determining if the element is in a modal and then getting the parent form object to set as the `dropdownParent `?

The attached patch is working for me in paragraphs, modals and regular forms without the need for a new wrapper element.