🇺🇸United States @ccjjmartin

This issue is attempting to fix the broken logic in the 2.0.4 version of the module, which I can verify is a real bug. However, the proposed change here will be in conflict with the direction taken in the 2.1.x version of the module, see this issue: https://www.drupal.org/project/block_visibility_groups/issues/2864027 ✨ Allow choosing more than 1 visibility group per block Needs work

There is a backport patch to the 2.0.x version of the module available there that modifies the same lines of code the proposed patch here is modifying. I believe the other patch fixes this issue as well and knowing it was already merged in the 2.1.x version, that in theory is the direction the maintainers was to head. Ideally someone who understands the code in this patch would review the affected code in the already merged branch to verify if the issue here was fixed or not.

Bug in the current functionality. I did not understand how the module works so I added a condition group to itself thus creating a recursive loading of itself infinitely and breaking the site due to an exhausted memory PHP failure. Recommend editing this patch to check the active page and remove the active condition group from the available condition groups within the dropdown.

Also, while these features could co-exist, a reminder for everyone on the thread that this was merged in the next minor version release of the module 2.1.x and may supersede this patch: https://www.drupal.org/project/block_visibility_groups/issues/2864027 ✨ Allow choosing more than 1 visibility group per block Needs work

Patch in #62 does not apply to 2.0.4

I had to capitalize the S in the Settings file. Updated patch to #19 provided.

ccjjmartin → created an issue.

g4mbini → credited ccjjmartin → .

Don't use either of the MRs or patches. A different solution entirely will be needed once an approach is determined.

ccjjmartin → changed the visibility of the branch 3511232-add-to-cart-2 to hidden.

Adding an attribute to the product seems to have resolved my immediate issues, a possible "fix" for this could either be some error handling feedback to the user to let them know attributes are required. Or have a fallback to the title handler if no attributes are provided by the user.

Looks like this is actually related to variation types with no attributes. Seems like the component type commerce_product_variation_attributes expects the variation types to have attributes in order to be shown. Even then though the cart is still broken and not displaying the variation attribute label.

The currently proposed MR appears to be breaking tests from the original parent issue, this will need some more work:

---- Drupal\Tests\commerce_cart\Functional\MultipleCartMultipleVariationTypesTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-63.xml       0 Drupal\Tests\commerce_cart\Function
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.45 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.17
    Configuration: /builds/issue/commerce-3511232/web/core/phpunit.xml.dist
    
    F                                                                   1 / 1 (100%)
    
    HTML output was generated.
    http://localhost/web/sites/simpletest/browser_output/Drupal_Tests_commerce_cart_Functional_MultipleCartMultipleVariationTypesTest-1-18769668.html
    http://localhost/web/sites/simpletest/browser_output/Drupal_Tests_commerce_cart_Functional_MultipleCartMultipleVariationTypesTest-2-18769668.html
    http://localhost/web/sites/simpletest/browser_output/Drupal_Tests_commerce_cart_Functional_MultipleCartMultipleVariationTypesTest-3-18769668.html
    
    
    Time: 00:23.784, Memory: 6.00 MB
    
    Multiple Cart Multiple Variation Types (Drupal\Tests\commerce_cart\Functional\MultipleCartMultipleVariationTypes)
     ✘ Add to cart
       ┐
       ├ Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "Color" not found.
       │
       │ /builds/issue/commerce-3511232/vendor/behat/mink/src/Element/TraversableElement.php:271
       │ /builds/issue/commerce-3511232/modules/cart/tests/src/Functional/MultipleCartMultipleVariationTypesTest.php:174
       ┴
    
    1 test triggered 1 deprecation:
    
    1) /builds/issue/commerce-3511232/web/core/modules/views/src/ViewsConfigUpdater.php:258
    The update to convert "numeric" arguments to "entity_target_id" for entity reference fields for view "profiles" is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Profile, module and theme provided configuration should be updated. See https://www.drupal.org/node/3441945
    
    Triggered by:
    
    * Drupal\Tests\commerce_cart\Functional\MultipleCartMultipleVariationTypesTest::testAddToCart
      /builds/issue/commerce-3511232/modules/cart/tests/src/Functional/MultipleCartMultipleVariationTypesTest.php:168
    
    FAILURES!
    Tests: 1, Assertions: 31, Failures: 1, Deprecations: 1.

ccjjmartin → changed the visibility of the branch 3511232-add-to-cart to hidden.

ccjjmartin → created an issue.

I am proposing a change to the code that was added in this issue. Would appreciate some more eyes on this bug to verify it doesn't break anything introduced here: https://www.drupal.org/project/commerce/issues/3511232 🐛 Add to cart form does not display certain variation types Active

Also as a note, I have products with multiple variation types and they display both variation types. So the intent of the original issue has not been broken by the proposed patch as far as I can tell.

As a note, I flagged this as the 2.40 version because that is the version I am testing with but I linked the code from the 3.0.x version because it is still present in the latest version of the module.

ccjjmartin → created an issue.

#5 fixes broken logic conditions for me, +1 RTBC

g4mbini → credited ccjjmartin → .

This is definitely a good start for what this module needs, there are some complex problems to solve left on this one. For the moment I added a workaround of adding this hook into my custom user theme:

/**
 * Implements template_preprocess_HOOK() for form_element.
 */
function hook_preprocess_form_element(&$variables) {
  $variables['description_toggle'] = TRUE;
  $variables['description_display'] = 'invisible';
}

The reason that the gin preprocess hook isn't triggered within Layout Builder is because the custom user theme is the active theme, not gin, so the workaround above makes sure that the necessary variables are set. I may also have some additional issues related to formatted text fields showing no description value to the preprocessor ... but yet somehow a description is still appearing?

@grimreaper I see this is still marked as needs work, what is still pending on this one? Your patch looks like what I was thinking about implementing for this so I think you are on the right track here.

Followed the steps in #4, patch works great for me +1 RTBC.

The latest version of Gin 3.x fixed this issue for me without adding the drupal_ajax check, I find that odd but there was quite a bit of rework on the logic here, so since things are working so I am going to mark this as resolved.

The latest version of Gin 3.x fixed this issue for me without adding the drupal_ajax, going to comment on the subsequent issue created that this is now fully resolved.

ccjjmartin → created an issue.

Can confirm that #29 fixed the issue for me.

+1 RTBC, noticed this earlier today.

This is related to upgrading to PHP 8.2. This patch fixes the issue for me. Marking as RTBC.

I can confirm that the only thing needed here is a new tagged release as the latest dev branch has the necessary required change. The release as of 4.0.3 did not have the required change in the composer.json file, notice the lack of or Drupal 10: https://git.drupalcode.org/project/radioactivity/-/blob/fa9651f23d6a5ee1...

Two small PRs to update the base theme from stable to stable9 for D10 compatibility.

ccjjmartin → created an issue.

I used the patch from comment 24 and fixed a conflict in the CHANGELOG.txt as well as adding one fix to a deprecated module_load_include() on merge request 7. My results using the upgrade status module show that this module is now fully compatible with D10.

For testing you can do the following since there is a change to composer.json you will need to make composer aware of the change and patches managed by composer won't do this, it must be done using a separate repository managed by composer. So you first edit your repositories section of composer.json to include a new git repository (above the exisiting drupal one so it is searched FIRST):

        {
            "type": "vcs",
            "url": "https://git.drupalcode.org/issue/forum_access-3410468.git"
        },

Then you edit the forum access require line to include the git repository branch name you want, this is done using:

        "drupal/forum_access": "dev-3410468-24",

Then, until the acl module has committed it's info.yml change, assuming you use the cweagans/composer-patches package to manage patches via composer, you can use the following to get the D10 patch, no repository change is necessary because no changes to composer.json are made:

          "drupal/acl": {
              "D10 compatibility": "https://www.drupal.org/files/issues/2023-12-21/acl_beta_1_drupal_10.patch"
          },

ccjjmartin → made their first commit to this issue’s fork.

ccjjmartin → changed the visibility of the branch 3414025-d10-updates to active.

ccjjmartin → changed the visibility of the branch 3414025-d10-updates to hidden.

ccjjmartin → made their first commit to this issue’s fork.

ltrainjohnson → credited ccjjmartin → .

Thanks everyone this has been merged.

ccjjmartin → made their first commit to this issue’s fork.

Thanks for the reminder to merge. Everyone on the thread got credit including the original mention of the user who suggested the tip originally. Thanks all.

ccjjmartin → created an issue.

https://github.com/fourkitchens/sous-drupal-project/pull/105

Adding patch and MR.

ccjjmartin → created an issue.

@d70rr3s thanks for your contribution, the changes work great, committing them so they get into the stable D10 compatible release.

ccjjmartin → made their first commit to this issue’s fork.

Looks good to me, merging. Thanks everyone.

Gábor Hojtsy → credited ccjjmartin → .

Gábor Hojtsy → credited ccjjmartin → .

Discussion of a stable release is in progress for D10.

info.yml has a conflict after the merge of D10 compatibility.

RTBC changes look good. Going to merge this and get a stable release out soon.

Thanks for this contribution, I can confirm this fixed my issue also. I was specifically using the empty option setting, looking in the description I see that empty value was also set to -1 and when I attempted that on my environment I got an error about an invalid option set. So I just removed that and only set the empty option and the patch works great.

I contacted nginex today as requested.

As a note, I left this issue open for well over the 2 week period with no response from the maintainers, it was closer to 2 months.

Adding paragraphs features module to the list of modules that needs work

ccjjmartin → created an issue.

This comment solved my issue, thank you.

The newest major version release of webform group (3.x) does work with the latest version of webform (6.2.x) and group (3.x).

@maskedjellybean the code looks fine to me, have you tried removing that code to verify that it is the problem? Seems like if $is_group_node is FALSE (the default behavior) that the code will run the node access check.

For reference, the reflections class error appears to be a non issue with this module and instead is a error in the drupal-phpstan: https://github.com/mglaman/phpstan-drupal/issues/143

This work appears to have been done in another commit somewhere, the latest version 2.0.2 has the info.yml patch for D10 compatibility in it already. Closing this issue.

Evan got back to me and said he did not have permission to add me as a maintainer, are any of the other maintainers able to do so?

@rigoucr I would consider removing the reference to Drupal 8 compatibility in the module (if you choose to keep it). I am not sure if the submodule was intended to be kept permanently or if it was for easy testing but it seems like this one hook could be placed into the module itself? Looking at the code it looks like we have a "crop" module that has a "crop" entity and when the crop entity is updated the image style isn't flushed?

hestenet → credited ccjjmartin → .

In theory this would be considered a breaking change so it may also be a good time to switch the module to using the new semantic versioning branches and create a 2.x branch with releases there.

ccjjmartin → created an issue.

Just to post an update, I updated to 2.x and it fixed the deprecations and the module still works. Thanks for the quick response.

ccjjmartin → made their first commit to this issue’s fork.

ccjjmartin → created an issue.

Looks like 2 of the 3 instances of this method were previously replaced and this picks up the last one.

ccjjmartin → created an issue.

I included this change in the Drupal 10 upgrade patch since Twig is upgraded from 2 to 3, recommending marking this one as duplicate.