🇨🇦Canada @joelpittet

@anybody if by chance you can review and test the solution fixes the problem that would help move it along. It's got tests to go with the direction I am hoping to take this.

It’s definitely beneficial to keep things separate if they truly are distinct. If this can be resolved independently, I’d recommend keeping it here. Though if they are tied at the hip, then keeping them in the same issue is preferred.

joelpittet → created an issue.

I added the PHP attributes (with the doctorine annotations for older 10.x support) probably scope creep here but I will try to keep that to a minimum. Removed even_empty which isn't used anywhere on the ViewsStyle

joelpittet → created an issue.

Yes, the $ogRole->label is that field to query. Sorry I am jumping around between a few projects at the moment, so my fancy ideas are even escaping me! The idea was to use a helper to generate the options list and provide a way look up which entity/bundle combo those labels would apply to when filtering in Views. Maybe not totally needed, but at the moment it seems like a good idea.

@ressa It definitely will get out of date eventually—no argument there. I may be able to wrap something like make lint into a ddev lint command, but I’ll probably keep them separate for now to avoid stepping on existing workflows.

The main benefit for me is having a consistent, isolated way to stand up this project for manual testing, without relying on a possibly patched version from another project or a contrib testbed with unrelated modules. I totally get the concern about long-term maintenance though, and I’m happy to revisit the setup if it becomes a burden. Though I am planning on keeping it up-to-date, so the burden is on me.

I've committed this to the dev branch for hopefully a quick release, thanks again @apkwilson and @daniel korte for bring this issue up and finding a solution!

Moved the duplicate-check logic into a new instance-based isEntityAlreadyRendered($id) method. This change was motivated by two goals:

1. Making the code much easier to test.
2. Simplifying the render() method by removing internal static logic.

Test highlights

1. First call with an ID → false
2. Second call with the same ID → true
3. New ID resets correctly → false, then true
4. Calling the first ID again later → true, confirming we now track all previously rendered IDs—not just the last one, which was the regression we fixed.

Real‑world verification
I manually reverted to the old static-based logic inside isEntityAlreadyRendered() and confirmed the last assertion failed. I also tested with a real date_recur scenario—this solution behaves identically to the static version but is hopefully easier to maintain and test.

I manually reverted to the old static-based logic inside isEntityAlreadyRendered() and confirmed the last assertion failed. I also tested with a real date_recur scenario—this solution behaves identically to the static version but is cleaner and easier to maintain and test.

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

Probably overkill but I want to get more tests in this module so getting the ball rolling here. Thanks for this, fix btw @apkwilson, it fixes a problem for me with date_recur I was seeing as well, so I will commit it really soon!

RE #23 I agree with you @imclean — re-scanning the entity on this manual task would make it significantly more useful. Just to confirm, which MR are you reviewing? Based on Joseph’s comment and yours, I’m guessing it’s MR !114, but want to be sure we’re all looking at the same thing.

Thanks for the thoughtful response! Totally fair—if the DDEV setup ever becomes neglected, I have no problem with it being removed. I’m happy to keep it up-to-date as long as I’m actively contributing here.

Also, thanks for the note about make lint. I hadn’t noticed that was already in place—very helpful. The DDEV setup shouldn’t interfere with that workflow at all, but I’ll add a note in CONTRIBUTING.md to clarify how it fits alongside the DDEV tooling.

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

Thanks again @dpi, I don't intend on adding too many features, mostly bug fixes, though I suppose if I need to do something drastic it would be worth creating a separate widget Bravo or Charlie 😉

joelpittet → created an issue.

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

I had a test and a fix that fixed this in 🐛 Decimal separator and decimals settings ignored when aggregating decimal fields Needs work and it got committed, but then someone realized it broke something else, so it got reverted... it looked like a copy/paste mistake given the commits around it, but in the end it wasn't.

In any case this may be a duplicate.

Thanks, @herved — you might be right here, especially since you probably have a better handle on the internals of ::optimizeGroup() than I do.

If you haven’t already, feel free to take a look at the test in 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active — it might be helpful to reuse or adapt parts of it to confirm whether ::optimizeGroup() is actually called in these cases. Even just running the test on its own could help show the problem is fixed. That kind of check might make the expected behavior a bit clearer.

I’m still a bit cautious about collapsing the two issues, since that overlap is partly what got us into this situation. But if you feel strongly that they belong together, I won’t block it. I do think the targeted fix in 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active should resolve the CSS side of things.

Really appreciate the work you’re doing here — you might end up solving a broader problem in the process! I’m just laser-focused on cleaning up the regression I introduced, and if I expand the scope too much, I’m afraid I’ll cause another one 😬

Yeah, I agree — OG requires at least one og_standard_reference field on group content to function properly.

How about: what if we instead throw a warning on the Field UI and/or Status page if a group content bundle doesn't have any og_standard_reference field defined? That way we’d catch real misconfigurations regardless of field name, without assuming a specific implementation or locking down unused fields.

That could provide the safety you’re looking for, while still keeping things flexible for cases like ours where the field exists under different names due to migration history (or other pedantic reasons).

I don’t recommend locking the og_audience field in our case.

While it’s an important default that OG creates when no other og_standard_reference fields exist — and definitely useful for new OG installs — our situation is different. We're migrating from Drupal 7 and already have several specifically named og_standard_reference fields in use. In our case, og_audience has been deleted and is not part of the current architecture.

Locking it could prevent us from removing or editing the instance/widget settings — for example, to use custom selection handlers. That gives me pause, especially considering the underlying ambiguity around the meaning and enforcement of “locked” in core config entities. Some relevant issues:

• 📌 Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing Active
• 🐛 Locked fields can change the widget but not settings Needs work
• 🐛 Do not allow to alter Locked field via UI Needs work
• 🐛 Locking field.storage should not lock field instance settings (field.field) Active

What problem or motivation is behind the request to lock it? Or are you assuming some of the issues above are unlikely to ever be resolved? I believe (though haven’t tested) that removing locked fields via config import might still be possible under current behaviour, but if some of those issues above are resolved that may not be the case in the future.

That’s a novel idea—definitely good to think of alternate approaches. That said, I’d prefer not to add custom fields just to filter roles by label. It feels a bit off to decouple the display label from the core role entity, and it adds extra overhead that’s easy to miss or forget about later. At that point, using a grouped exposed filter in Views might be simpler, even if it requires some relabeling.

What I was originally thinking was something like adding a method to OgRoleManagerInterface, maybe getRolesByLabel(), to return a unique list of roles keyed by label regardless of bundle. But maybe there’s some resistance to that since it could introduce ambiguity or complicate the API? Curious what your take is?

The current MR shows what's in the screenshot in comment #3 BTW

Thanks for setting such clear expectations—really appreciate the thought you’ve put into it.

Just to reassure you: I don’t plan to make any big changes without checking in first. For smaller fixes or features with tests, I’ll likely commit directly. With Views-specific changes, I’ll make a best effort to include tests, but if it starts getting too sanity-draining, I may commit without them (with discretion, of course).

I’ve worked with multilingual at a surface level—some in contrib, some in core dev—but only a few actual projects. One was even just US vs. Canadian English, so… not exactly a stress test 😅 Still, happy to help there where I can.

Tests (especially for Views) are something I struggle with too, but I’m actively working to improve on that. I’ll do my best to keep the 3.8.x branch green—mainly because I need it for a D7 to D10 migration (blocking D11 until dependencies are sorted). Eventually I’ll relax that, like you mentioned.

As for big picture stuff, I’ll try to surface it openly in issues, though I may check in over Slack first if something needs a quick sanity check before wider discussion.

Lastly, I just want to say I’ve noticed and appreciated the attention you’ve given to timezones in the past (recalling from previous issues), and the architectural choices—especially the split between the date field and occurrences table in the DB. I think it's a nice setup and inverse of D7’s date_repeat and D8+'s smart_date database structure.

joelpittet → created an issue.

joelpittet → created an issue.

joelpittet → created an issue.

I love the idea but I think maybe there are better versions nowadays. I have been liking Vite's live reloading in my Laravel projects. And there might be a few tools that got it working with Drupal worth trying out. In that case it would be more a hey, we recommend going to use Basic with X project.

https://github.com/segovia94/drupal-vite-poc
or
https://www.drupal.org/project/vite →
or
https://www.npmjs.com/package/vite-plugin-drupal-template-hmr

I haven't used any with Drupal (yet) but kinda want to, though I loath dependencies... so won't play with it for a while.

We have various views with exposed role filters—like filtering for who’s the chair of a committee, or who’s an administrator of a group. In some cases, we also want to filter by any role except "Hidden" or "Portfolio” (that’s a real example). From a user’s perspective, they don’t care which group type (bundle) a role is attached to—they just want to filter by the Role (label).

I’d prefer not to prefix the role labels with the bundle to distinguish them to the site builder configuring this, and since that would mean using grouped exposed filters and manually relabeling everything for clarity, which adds unnecessary complexity. So I guess I am leaning towards the helper to lookup roles by label and just showing (unique) labels like D7 had.

Feel free to disagree if that doesn't make sense still

@dalin I tried my hand to add some more tests for a few of these cases but I didn't want to step on @benabaird's toes so I put it on a new branch MR 12687 with slightly different take on how to solve this and some more comments.

@benabaird I hope this works for you?

It shouldn't add a fieldset actually, I see you have twig debug turned on in your issue summary comments, could you add the comments for which template is loaded for the wrapper that includes the fieldset?

My guess is a theme is overriding datetime_wrapper, but not totally sure without that context. Thanks for following up.

@herved Thanks so much for jumping on this — I’m seeing the same thing in my logs, so really appreciate you taking this on!

RE The mention in the proposed solution

This would also cover 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active which is a consequence of the change in #3414173.

I wonder if it might be helpful to keep the scope of that one in its own issue, just to keep things a bit more focused? I realize they are related (because this JS one is where I copied from in 🐛 Website error Exception: "Only file CSS assets can be optimized" Active ) but painting them with the same brush is what brought us here in the first place.

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

joelpittet → changed the visibility of the branch 11.x to active.

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

@herved and @benabaird I see what you mean—sorry to cause so much trouble with this, and thanks for following up (and for opening a new issue to track it).

I ran into the same thing with fontawesome as you did, @benabaird. When I wrote the patch and test, I copied how JS assets were handled, and there wasn’t anything in the comments or tests at the time to highlight this gotcha for CSS files—especially for local minified files relying on URL rewriting during aggregation.

I wouldn’t want this commit reverted, as it does solve a different and legitimate problem. But I do agree that targeting external assets specifically would be a more precise fix, and wouldn’t break local asset aggregation that people are relying on.

Appreciate you both raising this and suggesting a focused fix. I will see you over on the follow-up issue!

Let's mark this as fixed as it was committed and in the latest release in this commit: https://git.drupalcode.org/project/date_ical/-/commit/018255e0a4d42de7fb...

@smustgrave, yes, I am working on tests at the moment.

Apologies for the earlier confusion—I briefly pushed and hid an MR while testing. I’ve opened 🐛 #states: alternative fix for datetime element Needs review with a cleaner branch that simply adds the same .js-form-wrapper / .form-wrapper classes used by <details> element. This keeps this long history intact (too many needs tags and this is getting dated due to postponed issues), so if you're interested in possibly a way simpler solution, please continue the review there and again sorry for the noise.

joelpittet → changed the visibility of the branch 2419131-datetime-states to hidden.

joelpittet → created an issue.

D7:
→

D10 (Work in Progress):
→

In D7, access was determined based on the role labels, which allowed targeting roles across bundles more easily.
In D10, I’ve shifted to using role IDs, but the tradeoff is that I can’t easily target roles across bundles unless I explicitly include each one.

Would it make sense to introduce a role name (label-based) lookup helper so that the behavior mirrors D7?
One concern with continuing to rely on role machine names is the potential for duplication—especially if roles are auto-generated per-node,
resulting in names like ENTITY_TYPE-ENTITY_BUNDLE-member and ENTITY_TYPE-ENTITY_BUNDLE-member-1.

Any suggestions or direction would be appreciated.

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

I believe this feature will get in to core eventually (I removed the double identification of the category in the title), so I’m on board with adding it to this module as well.

The approach is important here—what do you propose?

If you start a WIP merge request, it’ll be much easier to discuss the direction in concrete terms but even a rough draft "proposed solution" helps get the conversation moving.

I'm simplifying the scope as it seems the relationship and field are actually working (my migration needs a bit of help for the roles apparently).

Thanks @amitaibu!

joelpittet → created an issue.

Might be related to 📌 Improve views support for date recur base fields Active

joelpittet → created an issue.

I went through the MR changes they look straight forward, I have committed similar on og and panels recently.

Ah I see, the \Drupal\Core\Form\ConfigFormBaseTrait::config() does some jumping through hoops if the config factory is not injected to grab it off the container. It does help if the property is already there, so injecting it seems the way to go from my prospective.

I love the simplification here — big +1 from me.

One thing I’m wondering about: with the new approach, if a plugin throws an exception during clean(), it looks like the remaining plugins won’t be called. Previously, each plugin’s clean was isolated inside its own listener. Is that an intentional change, or should we consider wrapping each plugin’s call in its own try/catch? I think the answer is that the catches happen deeper anyways so not a concern but thought I'd ask anyway.

Also, is there any risk that one plugin’s clean() call could mutate shared state on the feed in a way that affects how later plugins behave within the same loop?

@amitaibu the test failure was fixed and committed here 📌 Remove explicit phpunit constraint to avoid CI failure on core 11.2.x-dev Active

Fixed merge train leaving the station!

Let's see if that fixes the issue...

joelpittet → created an issue.

@amitaibu sure thing, just got back from mini vacation!

→

Thanks @nickdickinsonwilde and others for taking a look. I have merged this into the dev branch.

@zviryatko Thanks for the clarification! Would you be open to helping move the issue forward by turning these patches into Merge Requests? That would make it easier for us to review and discuss the code more effectively. Also, feel free to join in that issues discussion and review it's MR to help move it forward.

Barring any merge issues, this should be fixed—thanks @nehajyoti! Also you almost had the MR working, just needed to create it, the commit was there and everything.

As a side note, I’d recommend moving to Layout Builder instead of Panels if you have the opportunity.

@herved Your expectation is how it should work, this change was creating parity in how CSS works as that is how JS works (I lifted the code from what we had in JS already). Can you revert the diff on your machine to see if it truely is this commit? (I was confused about "minified" coming into this issue in the first place, I think I got a better grasp coming out.

You can grab the commit URL and add .diff to the end to get a patch and reverse it like this:
curl https://git.drupalcode.org/project/drupal/-/commit/a1b3926d59e341d771963eb8a3de9d881be7a107.diff | patch -p1 -R
And after testing it re-patch
curl https://git.drupalcode.org/project/drupal/-/commit/a1b3926d59e341d771963eb8a3de9d881be7a107.diff | patch -p1

Ok I think I got this solid, could use some eyes.

@alan ridgway Thanks for the report. A white screen usually means there's a PHP error being suppressed. Can you check your logs or enable error reporting locally and share the specific error message?

To enable error reporting, you can add the following to your settings.local.php:

$config['system.logging']['error_level'] = 'verbose';

Once enabled, try reproducing the issue again and let us know what error appears. That will help us pinpoint the cause.

Thanks for the write-up on the issue. Which hook are they using, does it provide the same functionality?

xjm → credited joelpittet → .

Echoing #97 claudiu.cristea — “I would say yes.” Drupal Core should ease the transition in 11.x and fully remove it in 12.x.

I don't grok what is suggested in #98 sorry @godotislate

@pdureau #95 can you point me to the technical debt you are speaking of, I was responsible for some of it, so I am curious what I broke in the future!

xjm → credited joelpittet → .

Let's close this as it's had it's day

I think coder dropped CSS recently, I will close this for stale clean-up

gitlab-ci is the successor, I don't believe this is relevant (cleaning up issues I was following)

gitlab-ci is the successor, I don't believe this is relevant (cleaning up issues I was following)

Is this issue trying to solve a similar problem?
🐛 TypeError: array_key_exists(): Argument #2 ($array) must be of type array, string given Active

Thanks for reporting this. Could you clarify the underlying problem you’re trying to solve?

The issue summary explains that the markup changes and that a fieldset is used instead of the core’s default form-element, but it’s not clear what the impact of that change is. For example:

1. Is the concern mainly visual consistency?
2. Does the current structure break theming or accessibility?
3. Are there usability issues with the new layout or behavior?

It would also help to explain how your proposed solution—using type="date" or datetime-local" while keeping the core markup—addresses that problem.

If possible, screenshots of the current vs. expected output or examples of where the new markup causes issues would make the case stronger.

Thanks!

Also note this issue may change the markup, depending on the outcome 🐛 Undefined array key "#type" in Drupal\Core\Form\FormHelper Active

@andreasderijcke re #8, can you elaborate this bug report in a new issue?

Closing this on #5's great response! Thanks @hubertinio

We will close this in a month without a response, otherwise just poking to clean up the queue.

The HTML5 date input doesn’t support partial dates like day/month without a year, so implementing this would be difficult without replacing the input type entirely. If you need this feature, it might be easier to fork the module and adjust the field type and query logic as needed.

Closing as Won’t fix.

Just changing the status to active as we don't have an MR yet (an cleaning up the queue)
https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

Postponing this on 🐛 Undefined array key "#type" in Drupal\Core\Form\FormHelper Active please see if the latest MR resolves this issue for you.

Thanks a bunch @ramil g!

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

Thanks @albeorte, I wonder how you tested that, it's a class __construct change that will have a regression
see this issue 🐛 [regression] The new property \Drupal\Core\Form\ConfigFormBase::$typedConfigManager conflicts with some contrib modules Fixed

Given this Change Record https://www.drupal.org/node/3404140 → we have a few options:

1. Create a new branch 4.x for Drupal core >=10.2
2. Set the minor version on 3.1.x to require Drupal core >=10.2

Setting to needs work because the base class constructor is not called any more in that, and so the config manager is not being set.

Merged this one-liner

Actually I think this is safe enough to move forward with, discussed with Graber in Slack and while he may not use it, it will hopefully make it easier for others to contribute back.