🇨🇦Canada @joelpittet

@anthonyroundtree it sounds like you did all the right things, thanks for detail your test! I am quite puzzled, I will try those too on a fresh DDEV instance later this evening.

I have functional tests in there too prove it's working but maybe they are wrong.

Is there a way in DrupalPod to tell if the Merge Request (MR) is applied to linkchecker?

Yes, this is my mistake, I blindly trusted this typehint:
https://git.drupalcode.org/project/calendar/-/merge_requests/60/diffs?co...

And that could very well be true that it sometimes is that value, if it's not the ->getStartDate() 3 lines lower will fail.

@anthonyroundtree that's a good way to test. The trick is you need to create content with bad links, run cron a few times to get it, look to see it's on the broken link report, then go edit that content and it should show in a warning at the top. (that is the hope)

Though I thought drupalpod had stopped working or maybe I am confusing it.

Always nice to move towards stable, though if any of the changes are problematic don't want to be too close to the bleeding edge... 🎲

Yes, please, patching it locally on a few sites, would be nice to remove the patches.

Awesome, thanks for giving the feature a second thought!

@ivnish I am good with committing this as-is. We could test session too but there are probably other tests already checking for that condition as we copied from other service methods. So 🚀

Ah thanks, well let's put this back to RTBC shall we ;) It wouldn't let me do the retest for some reason... maybe too used to the permissions on other modules

@liam morland the test failure doesn't appear related, is there anyway to be sure that this is not an unrelated test failure (I've been seeing a bunch that I had to fix in my projects). I will re-run the test but I wonder if we can see branch tests too to confirm it's not related.

Webform Element Fieldset (Drupal\Tests\webform\Functional\Element\WebformElementFieldset)
✘ Fieldset
┐
├ UnexpectedValueException: RecursiveDirectoryIterator::__construct(/builds/project/webform/web/sites/simpletest/20218150/files/config_jlGIIYQXrRYiCCtLL3L6hUnoOMsAkNqPvbnQeB8Ff3Z7dmdA3f_Mi6wt8dXzixhMgaWDnJEIjw/sync): Failed to open directory: No such file or directory
│
│ /builds/project/webform/web/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:169
│ /builds/project/webform/src/Plugin/WebformElementManager.php:77
│ /builds/project/webform/web/core/lib/Drupal/Component/DependencyInjection/Container.php:259
│ /builds/project/webform/web/core/lib/Drupal/Component/DependencyInjection/Container.php:177
│ /builds/project/webform/web/core/lib/Drupal/Component/DependencyInjection/Container.php:430
│ /builds/project/webform/web/core/lib/Drupal/Component/DependencyInjection/Container.php:463
│ /builds/project/webform/web/core/lib/Drupal/Core/Plugin/CachedDiscoveryClearer.php:55
│ /builds/project/webform/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:353
│ /builds/project/webform/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:229
│ /builds/project/webform/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
│ /builds/project/webform/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:511
│ /builds/project/webform/web/core/tests/Drupal/Tests/BrowserTestBase.php:537
│ /builds/project/webform/web/core/tests/Drupal/Tests/BrowserTestBase.php:343
│ /builds/project/webform/tests/src/Functional/WebformBrowserTestBase.php:44
┴

ERRORS!
Tests: 1, Assertions: 0, Errors: 1, PHPUnit Deprecations: 2.

I added populateFlaggerDefaults() because I think we were missing that, and removed the $flag because that use-case is covered by the other issue. Moving to Needs Review, maybe you can have a look @ivnish?

Thanks for retargetting the code in MR 49 to 5.x branch. I will put my proposal in MR 156 since they are now both on 5.x

@ivnish woah, you’re quick! I was juggling branches since I couldn’t retarget the MR to 5.x, so I created a new one and moved the code over.

Totally up to you, but my thought is to keep the signature specific to “all flags for user,” so the distinction between the two methods stays clear.

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

@ivnish, I think this one’s actually a different use case, so reopening as RTBC 🙂
The diff:

• getFlagUserFlaggings($flag, $user): Only returns the user’s flaggings for a specific flag.
• getAllFlaggingByUser(?$account, $flag = NULL, $session_id = NULL): Returns all flaggings for a user, with extra handling for anonymous users via session ID.

Since getFlagUserFlaggings() already covers the per-flag case, maybe we could simplify getAllFlaggingByUser() to just handle “all by user” and drop the “optionally narrowed by flag” bit? That way each method has a clear, distinct role.

Looks like you’re targeting 5.x, so I’ve moved it over there.

Removing the needs tests because we added tests, this could have a few eye balls (including my own) to review.

I am not sure how to make my stuff work in a generic way. The code in #2 works in that specific flag case, though so theirs a promise/hope.

Way too many issues on level 2 (mostly test related) that I will save that fun for a follow-up.

I don't know why I ignored the phpstan.neon in the first place...

Also related to 📌 Resolve PHPStan Errors and PHPUnit 10 Compatibility Issues in Drupal 11+ Active

I am going to dial up the PHPStan level slowly here (scope creep I know) but I think it's important to catch more real issues.

I will address these issues too.

------ ------------------------------------------------------------
  Line   og/og.module
 ------ ------------------------------------------------------------
  53     Variable $owner in empty() always exists and is not falsy.
 ------ ------------------------------------------------------------

 ------ -------------------------------------------------------------------------------
  Line   og/src/Plugin/Validation/Constraint/UniqueOgMembershipConstraintValidator.php
 ------ -------------------------------------------------------------------------------
  43     Variable $value in isset() always exists and is not nullable.
 ------ -------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------------
  Line   og/src/Plugin/Validation/Constraint/ValidOgMembershipReferenceConstraintValidator.php
 ------ ---------------------------------------------------------------------------------------
  33     Variable $value in isset() always exists and is not nullable.
 ------ ---------------------------------------------------------------------------------------

 ------ ------------------------------------------------------------------------
  Line   og/src/Plugin/Validation/Constraint/ValidOgRoleConstraintValidator.php
 ------ ------------------------------------------------------------------------
  20     Variable $value in isset() always exists and is not nullable.
 ------ ------------------------------------------------------------------------

Related to 📌 Support Drupal >=10.3. Modernize the code Active

joelpittet → created an issue.

Thanks for the plugin id and tests @ramil g.

I probably need to look at this deeper because with the relationship the base table might be moving around a bit so relationship plugin needs to adjust. I will stare at it some more...

This just needs to go in :D Thanks for those involved!

Thanks for posting the screenshots with the steps @franceslui!

Changing to Needs review for the patch. It would be nice to move this to an MR as well for easier code review.

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

@claudiu.cristea I took your comment in #3501637-13: Add a index table for group to group content relationship → to heart and gave it a try.
This is still a WIP, but I’d appreciate your review:
https://git.drupalcode.org/project/og/-/merge_requests/48#note_569723

I’ll do a self-review as well, and might also add a relationship and/or a regular filter so this “super UNION” can be reused in more "contexts" (overloaded pun intended).

For reference, the generated UNION is a bit gnarly 😅 — I know it’ll be slower than using an index, but in practice it might be perfectly fine.

WHERE ("node_field_data"."nid" IN (SELECT DISTINCT "t"."entity_id" AS "entity_id"
FROM
{node__og_group_ref} "t"
WHERE "t"."og_group_ref_target_id" IN ('60', '40218', '41304', '58943', '64777', '70707', '10', '70319') UNION SELECT DISTINCT "t"."entity_id" AS "entity_id"
FROM
{node__og_group_ref_also} "t"
WHERE "t"."og_group_ref_also_target_id" IN ('60', '40218', '41304', '58943', '64777', '70707', '10', '70319') UNION SELECT DISTINCT "t"."entity_id" AS "entity_id"
FROM
{node__field_user_groups} "t"
WHERE "t"."field_user_groups_target_id" IN ('60', '40218', '41304', '58943', '64777', '70707', '10', '70319') UNION SELECT DISTINCT "t"."entity_id" AS "entity_id"
FROM
{node__field_group_parent} "t"
WHERE "t"."field_group_parent_target_id" IN ('60', '40218', '41304', '58943', '64777', '70707', '10', '70319') UNION SELECT DISTINCT "t"."entity_id" AS "entity_id"
FROM
{node__field_academic_group} "t"
WHERE "t"."field_academic_group_target_id" IN ('60', '40218', '41304', '58943', '64777', '70707', '10', '70319') UNION SELECT DISTINCT "t"."entity_id" AS "entity_id"
FROM
{node__field_course_term_og} "t"
WHERE "t"."field_course_term_og_target_id" IN ('60', '40218', '41304', '58943', '64777', '70707', '10', '70319')))

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

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

Don't mind me just changing the version to the branch ;)

Thanks @dotpex for the patch, I hope I attributed you correctly, next time if you can make an merge request (MR) it would save me trouble trying to attribute you in the commit!

That sounds rough — permissions can definitely be tricky. I’m guessing you may have already tried this, but just to confirm: when you’ve uninstalled either og_access or content_access, did the expected results come back? That could help confirm whether it’s the interaction between the two or something else.

A couple of things that might help narrow it down:

1. Run drush php-eval 'node_access_rebuild();' after changing module combinations.
2. Check the hook_node_grants() output from both modules to see which one’s taking precedence.
3. Temporarily disable one module’s hook_node_grants() to confirm the other is working as expected.

And last question, are you using 2.x or 1.x of OG? I just split this modules branches based on that because of some type hinting incompatibility.

I'm retargetting this and opened a 2.x branch to get this sorted

joelpittet → created an issue.

I assume this is with OG 2.x?

This test failure seems unrelated — it’s complaining about a missing schema definition for click_sort_column, but that property is already used in many places in core. A bit annoying to have to fix this, and I’m not sure whether it should be addressed here or spun off into a separate follow-up, though a quick issue search talks about it here 🌱 Make views consistent in code, schema, tests, and APIs Needs review .

There should be no errors in configuration 'views.view.test_comment_last_updated_click_sort'. Errors:
├ Schema key views.view.test_comment_last_updated_click_sort:display.default.display_options.fields.comment_entity_statistics_last_updated.click_sort_column failed with: missing schema

Add regression tests for comment statistics sort with and without comment supported node types, so we don't need to do that later.

This is looking good. Due to the changes not applying in D10, it’s trickier for me to test manually right now.

I’ve postponed 🐛 Views ‘last updated/commented’ field fails to sort entities without comment statistics Active on this, it’s related but not the same issue, just commenting for awareness. It's related to the stats not being populated in D8+

I will postpone this on 🐛 Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column Needs work because it will likely conflict.

But a workaround in the mean time if anybody else needs this for table click sorting.

modules/custom/CUSTOM_MODULE/CUSTOM_MODULE.views.inc

/**
 * Implements hook_views_data_alter().
 */
function CUSTOM_MODULE_views_data_alter(array &$data):void {
  // Replace the core comment CES "last updated" sort handler with our NULL-safe version.
  if (isset($data['comment_entity_statistics']['last_updated']['sort'])) {
    $data['comment_entity_statistics']['last_updated']['field']['id'] = 'comment_ces_last_updated_nullsafe';
    $data['comment_entity_statistics']['last_updated']['sort']['id'] = 'comment_ces_last_updated_nullsafe';
  }
}

modules/custom/CUSTOM_MODULE/src/Plugin/views/field/NullSafeStatisticsLastUpdated.php

namespace Drupal\CUSTOM_MODULE\Plugin\views\field;

use Drupal\views\Attribute\ViewsField;
use Drupal\views\Plugin\views\field\Date;

/**
 * NULL‑safe field for newer of last comment / entity updated.
 */
#[ViewsField('comment_ces_last_updated_nullsafe')]
class NullSafeStatisticsLastUpdated extends Date {

  public function query() {
    $this->ensureMyTable();
    $entity_type = \Drupal::entityTypeManager()->getDefinition($this->getEntityType());
    $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);

    $this->field_alias = $this->query->addField(
      NULL,
      "GREATEST({$entity_data_table}.changed, COALESCE({$this->tableAlias}.last_comment_timestamp, {$entity_data_table}.changed))",
      $this->tableAlias . '_' . $this->field
    );

  }
}

modules/custom/CUSTOM_MODULE/src/Plugin/views/sort/NullSafeStatisticsLastUpdated.php

namespace Drupal\CUSTOM_MODULE\Plugin\views\sort;

use Drupal\views\Attribute\ViewsSort;
use Drupal\views\Plugin\views\sort\Date;

/**
 * NULL‑safe sort for newer of last comment / entity updated.
 */
#[ViewsSort('comment_ces_last_updated_nullsafe')]
class NullSafeStatisticsLastUpdated extends Date {

  public function query() {
    $this->ensureMyTable();
    $entity_type = \Drupal::entityTypeManager()->getDefinition($this->getEntityType());
    $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);

    $this->field_alias = $this->query->addOrderBy(
      NULL,
      "GREATEST({$entity_data_table}.changed, COALESCE({$this->tableAlias}.last_comment_timestamp, {$entity_data_table}.changed))",
      $this->options['order'],
      $this->tableAlias . '_' . $this->field
    );
  }
}

Looks like the same code is changing in 🐛 Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column Needs work — maybe this is a duplicate?

I feel the title represents something similar to an issue I am having, but I’ll create a separate issue to be safe (and not step on toes) 🐛 Views ‘last updated/commented’ field fails to sort entities without comment statistics Active .

joelpittet → created an issue.

The duplicate set description is already solved on dev

joelpittet → created an issue.

Just a caveat, maybe worth fixing here too, you need the URL to absolute, or validation when generating the View

joelpittet → created an issue.

Please have a look, I think this accounts for all the things.

joelpittet → created an issue.

Let's move this back to fixed as there is no response after #9 since Feb

I would recommend date_recur → (bias as co-maintainer) but also smart_date → , different approaches to the same problem.

The tests were written by AI, but I reviewed them, manually tested the code, and went through a few iterations to be transparent about the process.

Here is a screenshot of the manual test in a production D10 site.
→

Thanks again @ruslan. @dylan donkersgoed did anything get missed from your stuff in 44? I am not entirely sure what you did by the comments.

I agree with @joseph.olstad this is starting to feel like scope creep. While adding a manual rescan for entity links (to catch false positives on already corrected links on the content entity) would definitely be useful, it's a larger change. What we have now is already valuable: it allows us to manually verify that a broken link is still broken, without waiting for cron.

@solideogloria @iamclean — I'm resetting the title to reflect the current scope. If we decide to include the rescan functionality, it should be as a follow-up to this patch. My preference is to get this in as-is and open a separate issue for the manual rescan enhancement to this. If you'd strongly prefer to combine them, I'd urge you to fork the current MR and explore that approach on top of what's here in a separate MR... but I find it a bit clearer to make them new issues.

I hope that helps and doesn't muddy the waters — feel free to disagree, I am not the maintainer here.

Went to town a bit on the Issue summary, hope that helps, not sure to move this to 3.x or not, but 2.1.x to start @eiriksm correct this if I am wrong.

Closing as a duplicate of 🐛 Report on Content Edit page Active

Thanks for getting this in!

Thanks for sharing, the patch seems to suggest that maybe we are stepping on our own feet here. At that point in the code was the time zone already sent.?

@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.