🇨🇦Canada @joelpittet

Since @catch reverted the other issue, I caught up the branch to see if it now passes as I assume is expected now.

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

Sorry if I sound defensive, I am in a rush to get out the door... I will look a bit later to see if I can understand a bit deeper what is going on between the two regressions. It feels like it needs to be some sort of fallback in my guess, but it is likely way more complicated than that.

@acbramley see the test we have here, there was a regression fixed here as well. The screenshot shows the field handler was copied to all the handlers where in D7 it was only on 2 (that's why it appears to be a copy/paste mistake, it wasn't copied over like that on any of the other related commits at the time, in my deep dive/hunt for the root of the problem). Removing them let the default shine through (EntityField in our case).

https://git.drupalcode.org/project/drupal/-/commit/d15e2ea581dac9e112f15...

The here is the field we are testing aggregation on (id).
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...

@catch, It looks like there may be a regression—thanks for catching that.

Ideally, we'd add more tests here to show the new regression, no? The test in here shows the regression we'd been living with for a while.

See the screenshot of the various problems this solves in #58 #2735997-58: Decimal separator and decimals settings ignored when aggregating decimal fields → on top of it looking to be a copy/paste mistake (I keep saying that but looking at the diff it might be hard to see what I am saying), I wish dawehner could confirm this... sigh

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

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

The formatting is a bit jarring but the code looks solid.

Related 📌 RenderElement is deprecated Active

That is reasonable and good to bump the version of Drupal up to 10.3 to coincide with this change.

joelpittet → created an issue.

That seems like a way forward here, thanks @joachim

joelpittet → created an issue. See original summary → .

joelpittet → created an issue.

Thanks for your help I finished off the last little bits here and committed this all.

Thanks, glad to be on the board of dependencies!

Thank you @anish.ir, I have merged this in.

📌 Add DDEV Drupal Contrib for ease-of-maintenance Active Hope you don't mind, I cloned this great issue summary for calendar.

Looks like this has been resolved, I did offer to co-maintain that module to help easier in the future. 📌 Offer to co-maintain Active

I know there are test failures. I will fix them in the parent issues. I made PHPCS more strict, copying from what we did in og → .

joelpittet → created an issue.

joelpittet → created an issue.

This is a quickfix but it needs a pager that reads the value too, though it is better than nothing as someone could write a Views argument or argument default plugin that would read from the query string.

Side comment in the issue summary I wrote ?month=2025-04 even though I know in D8+ the month arg is in Ym format. I would like to see this easily configurable because ?month=202504 is practical but not a great hackable/human parseable URL

joelpittet → created an issue.

Thanks @claudiu.cristea!

Reviewed about half of the files touched here, caught a couple of things and they were addressed. This is good enough to be committed to the dev branch as all the big things are addressed and lots of huge improvements. Anything missed can be caught in production 🚀 🫣

Thanks @claudiu.cristea for taking this on!

Needs review from a path forward/proposal perspective, I know it needs tests still.

joelpittet → created an issue.

Could use a second pair of eyes, but this simple changes makes a big difference.

joelpittet → created an issue.

Back to RTBC, the problem I ran into was a separate issue, this MR doesn't affect submissions, I will create a separate issue for that.

joelpittet → changed the visibility of the branch 8.x-1.x to hidden.

joelpittet → changed the visibility of the branch 2.0.x to hidden.

Have one issue where the first column misses values (most of the time, I saw one numeric one migrate fine).

Fix seems to work.

Here's an example config export diff after it works, that is the way the current webform exports this.

diff --git a/config/sync/webform.webform.webform_838.yml b/config/sync/webform.webform.webform_838.yml
index 8474014b2..608564abe 100644
--- a/config/sync/webform.webform.webform_838.yml
+++ b/config/sync/webform.webform.webform_838.yml
@@ -149,16 +149,17 @@ elements: |-
             checked: true
   current_employer:
     '#type': textfield
-    '#unique': false
-    '#title_display': inline
     '#title': 'Current employer'
+    '#title_display': inline
     '#states':
       visible:
-        - ':input[name="current_employment_status[industry]"]':
+        - ':input[name="current_employment_status[Industry]"]':
             checked: true
-        - ':input[name="current_employment_status[academia]"]':
+        - or
+        - ':input[name="current_employment_status[Academia]"]':
             checked: true
-        - ':input[name="current_employment_status[startup]"]':
+        - or
+        - ':input[name="current_employment_status[Startup]"]':
             checked: true
   current_job_title:
     '#type': textfield

Note the keys aren't lowercased because that is not correct and has been corrected here as well.

joelpittet → created an issue. See original summary → .

The reroll is quite tricky because the logic for yaml creation changed... I am moving this back

joelpittet → changed the visibility of the branch 8.x-1.x to hidden.

Let's see this on 2.x

joelpittet → created an issue.

Using this with 2.x

It would be so nice if the tests didn’t change. Are the test changes really necessary? Adding more expected assertions is fine, but changing the provider values reduces confidence in the change.

Thanks for picking this up, it's been sitting for a while.

We don't need to bump Drupal here as if 2.x is asked for composer will do the requirements there for Drupal 10.3. So I will leave this commit simple.

joelpittet → created an issue.

I would say this adds lots of complexity for little value, I would suggest closing as won’t fix. Consider deleting would have no rows to select for example. I’ll let Graber decide though, he might have another take

Could you tell me what issues you have worked on for this?

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

I cherry-picked the version into the 2.x branch to move the needle a bit, I want to get some constraints using PHP attributes.

Thanks moshe in slack for pointing me here and thanks @herved for the workaround in #203!

I was getting
Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Save" not found.
Which was a FieldException if you looked at the page content

The website encountered an unexpected error. Try again later.

Drupal\Core\Field\FieldException: Attempted to create, modify or delete an instance of field with name og_audience on entity type node when the field storage does not exist. in Drupal\field\Entity\FieldConfig->getFieldStorageDefinition() (line 323 of core/modules/field/src/Entity/FieldConfig.php).

Drupal\og\OgGroupAudienceHelper->getAllGroupAudienceFields() (Line: 48)

joelpittet → changed the visibility of the branch 8.x-1.x to hidden.

Thanks for the call today. I realized during the call og_non_member_post was custom in D7 on our site, so we can just implement that.

We will handle the bug (I think @ramil g did it already) in 📌 OG reference field widget update: MR #570 did not properly merge, leaving og_complex in place Active .

I will close this as works as designed.

It should pass the tests and merge, closing as fixed.

joelpittet → created an issue.

Thanks for catching that, I have committed the code.

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

The callback quicktabs_machine_name_exists error in comment #2 was fixed here 🐛 all_user_func(): Argument #1 ($callback) must be a valid callback, function "quicktabs_machine_name_exists" not found or invalid function name in call_user_func() ( Active

The callback quicktabs_machine_name_exists error in the issue summary was fixed here 🐛 all_user_func(): Argument #1 ($callback) must be a valid callback, function "quicktabs_machine_name_exists" not found or invalid function name in call_user_func() ( Active

Thanks @smustgrave for trying to triaging this queue, and trying to reproduce the bug, also appreciated.

Same solution we are using in QuickTabsInstanceEditForm, and the function quicktabs_machine_name_exists doesn't exist so obvious oversight along the way. Thanks for the fix @tibezh.

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

@osopolar that’s a really good point — I should know better than to make assumptions about how this works. I was under the impression minified was an action to be taken rather than a descriptor. But you’re right, the docs clearly state otherwise. Looks like I’ve been shooting myself in the foot here! Thanks so much for pointing it out.

That said, I still think the page-cached asset URLs are a problem, regardless of my incorrect understanding. I’ll try to update the issue summary to reflect that.

Yes and no:), we have language enabled because search_api or the solr extension requires it but we are doing that one to one, und to en.

Do you think that is the ticket?

@amitaibu alternative I discussed with @ramil g, we can force the og:default in the field UI, and maybe strip out the option to select views or other ER entity reference selection handlers that are inherited? Then we create a separate OG specific handler for og_non_member_post.

For illustration purposes this is what I am talking about regarding this bug.

Before save:
→

After save:
→

D7
→

@amitaibu, thanks for the context. Just to clarify, when the field UI saves, it saves/exports the default:node selection handler, when it's created it's og:default. Although you mentioned it might be a bug, I believe it’s working as intended—that it should export and create the handler that way (currently creates the field as og:default).

The result would be that the audience field is created with a new handler that only allows selecting groups where the user is a member (with the OG admin exceptions), while default:node continues to work like the D7 og_non_member_post. This approach keeps the logic simple and ties it directly to the site builder’s explicit choice. What do you think?

Thanks for the context, @catch—much appreciated! I was pretty sure it would get in eventually, just didn’t want it to be one of those “not really RTBC” cases.

I’m really glad this bug is finally squashed—I kept running into it with aggregation enabled on all my migrations!

Thank you both and sorry for the delay, I just spotted this today.

@berdir Making the references to the preprocess in the Twig file is important to indicate the alter point.
That said, I prefer using the new/recommended approach while still preserving the old method. This serves as implicit documentation, helping those extending it understand how to do it going forward properly.

So +1 for what you've been doing from what I see at a glance. /Theme system maintainer hat 🎩/

joelpittet → created an issue.

I've merged this into 2.x branch. All the tests and some quick manual tests show it's working the same as it did before.

Thanks for reporting this, the proposed solution is good, it’s likely coming from an upgrade where we didn’t provide defaults in a hook or otherwise

joelpittet → changed the visibility of the branch 8.x-1.x to hidden.

To reiterate, this issue was introduced accidentally due to a copy/paste error, and it has been there for quite some time.
https://git.drupalcode.org/project/drupal/-/commit/684b4a036e736b16d21a1...

joelpittet → created an issue.

@heddn thanks for adding checkboxes support, any idea why it's not whitelisted and implicitly unsafe in the tests? Scouring the code didn't help discern why.

It looks like something changed the useage needs a value of 'true' now
prepopulate/src/Populate.php:137

        case 'checkbox':
          $form['#checked'] = $value === 'true';

So that would be
edit[field_name][widget][123]=true

We might need to do what @kerasai and extend PaymentInformation because we can get a free order with the promotion but trouble when the product is price 0 to begin with.

Sorry you did this already, I must have been looking at the wrong branch. It seems like the branches are a bit wonky in here btw, this MR is ahead and behind and also on 3.4.x all at the same time, won't update the branch for some reason

joelpittet → created an issue. See original summary → .

RE #14 This commit might be correct considering the comment I removed. https://git.drupalcode.org/project/og/-/merge_requests/19/diffs?commit_i...

@amitaibu and @claudiu.cristea,

While working on this, I’m not finding “real” permissions. The code isn’t detecting ‘administer group’, and it’s getting removed in OgRole::calculateDependencies().

This line: $permission_definitions = \Drupal::service('user.permissions')->getPermissions(); isn’t returning the expected results.

I’m unsure about the intended behaviour—are we relying on Drupal’s permissions, or are we using our own? If it’s the latter, I can dig in further, but I’m currently hitting a wall. Any insights?

Fixed that caveat by trimming the raw value to remove whitespace before and after the values.

Maybe it’s not being encoded and/or decoded with urlencode()?

Probably need to step through

@nicxvan I had a look, trying it out IRL right now. The code looks good to me, going to mark this RTBC from my end as it does what is on the Tin ;)

Already in both branches as it was committed before the creation of the 2.x