🇨🇦Canada @joelpittet

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

I set this as a support request instead of feature request because maybe I am overlooking an obvious way to switch out referenced products.

joelpittet → created an issue.

Yeah I am ok not backporting this.

@mikelutz 🐛 Fix migration issue for link fields with ‘External links only’ enabled Active fill your boots ;)

joelpittet → created an issue.

Though I got a notice while running the migrations to fix this issue, it worked great and solved the problem of the WebformLikeRt errors.

New notice was:
[warning] Undefined array key 1 D7Webform.php:991

Not a huge deal, but links meant to be external-only are getting and can’t be saved.

I just extended the functionality and added a revert to handle it. 🚀

use Drupal\link\Plugin\migrate\process\FieldLink as DrupalFieldLink;
use Drupal\migrate\Attribute\MigrateProcess;

#[MigrateProcess('my_field_link')]
final class FieldLink extends DrupalFieldLink {

  protected function canonicalizeUri($uri) {
    $uri = parent::canonicalizeUri($uri);
    if ($uri === 'route:<nolink>') {
      return '';
    }

    return $uri;
  }
}

Not sure how to check if external only is on the field settings, but this works to get the migration done

Sorry these issues are too disruptive to bugs in the queue, don’t spend any time on code standards issues 🙏

Thanks for the review!

I’ll look at it tomorrow when I am in.

This really looks like a duplicate of what is in 📌 Support Drupal >=10.3. Modernize the code Active , am I miss reading or should it be closed as a duplicate?

Ok this is working for me now. My last commit got rid of isHookDisabled() calls without the new $module arg because it wasn't available without implementing getImplementationInfo() and buildImplementationInfo() and verifyImplementations() which isn't the end of the world but getting pretty complicated and likely not needed.

The alter methods are tricky, because they call some of the decorated methods inside themselves. It’s almost better to just leave them alone.

Ah a Drupal acronym I've never heard of, thanks!

Thanks @nicxvan, I am not sure what HMIA is, I am guessing it's related to inheritance or something but google/ChatGPT didn't turn up good results.

@yaqbick Could you make this a merge request please? Also before you do that can you have a peek at the work done here: 📌 Support Drupal >=10.3. Modernize the code Active

Re #12, based on the Issue Summary goal of “let’s also remove the dependency on Drush,” I’m assuming we can remove the command entirely.

Since we are decorating the module handler, we also don’t need migrate_boost_module_implements_alter().

I’ll go ahead and remove them as I’m just trying to get this working. Let me know if I’ve overstepped in my haste!

RE #11, this is the error, I didn't make a commit because I don't have an idea what is supposed to happen with that, I suspect remove migrate_boost_module_implements_alter?

Error: Call to undefined method Drupal\migrate_boost\MigrateBoost::alter() in /var/www/html/public/modules/contrib/migrate_boost/migrate_boost.module on line 14 #0 /var/www/html/public/core/lib/Drupal/Core/Extension/ModuleHandler.php(552): migrate_boost_module_implements_alter()

Oh wow, thanks @ivnish for wrapping this up!

MigrateBoost::isBoostActive() seems to be always not set. And the drush command doesn't seem to be available.
I think the command is missing a services yaml like:

  migrate_boost.commands:
    class: Drupal\migrate_boost\Commands\MigrateBoostCommands
    tags:
      - { name: drush.command }

and I think the namespace for the command should be
namespace Drupal\migrate_boost\Commands; instead of namespace Drupal\migrate_boost\Drush\Commands;

Thoughts?

The MR still has this alter() static method call which doesn't exist any more:

function migrate_boost_module_implements_alter(&$implementations, $hook) {
  MigrateBoost::alter($implementations, $hook);
}

What's the plan for this?

I saw the same problem in 2.x

Sorry, I applied it against 1.x, disregard my comment

Same in the title, drupal 10 and drush 13

In CheckCircularReferencesPass.php line 81:

  Circular reference detected for service "migrate_boost.module_handler", path: "migrate_boost.module_handler -> config.factory -> config.typed -> migrate_boost.module_handler".

Current code produces this error. I tried uninstalling and pre patch, and installing with patch with the same result.

Can you create a Merge request instead of patches so we can consider it to commit to the dev branch?

I’m sure rerolling this patch is a thankless task

Wow, that was quick! Were you already working on the test, @b_sharpe?

I always hesitate to modify an existing test instead of adding a new one since there’s a risk of losing valuable coverage. It’s a bit tricky to tell if anything important is lost here, but seeing the red/green tests is definitely encouraging! Can you alleviate that worry?

Does what is on the tin ;)

The code differs significantly from the 2.1.x branch. Adding tests would help ensure this bug is fixed in both branches.

@cobenash, @proweb.ua, @ccjjmartin – would any of you be able to write a test to prevent regressions? A merge request would be preferred, but I appreciate the patch!

This looks good, just running the tests only on the latest to see if they fail as expected. Thanks for putting this together with a test no-less.

RTBC++ this indeed critical and has a test, nice work! @andras_szilagyi

Is that because of this issue 🐛 Missing plugins in frequency Active ?

Thanks @darren.fisher. And a bit of a silly question but ui feature is not good enough to solve this problem (see screenshot)

→

@claudiucristea I am +1 to make a 2.x release too, does that work for you too?

RE

Read-only class properties

I backed out of them in this issue because of a serialization problem 🐛 Error: Cannot initialize readonly property Drupal\og\Plugin\Field\FieldWidget\OgComplex::$membershipManager Active

I’ll also be working on a large menu for an intranet soon, so I’ll definitely test this out. Thanks for the detailed response!

Ideally, we can find a solution that makes everyone happy. If there’s a relatively small change (like the one you made by switching conditions), those are easy wins, though they might not fully align with what you’re aiming for.

This is a quick fix, it should auto merge when it passes.

Split off the UI issue here so it can be addressed independently
🐛 UndefinedLinkTemplateException when adding a new OG role in the UI Active

joelpittet → created an issue.

Fixed, closing. If anybody is interested feel free to open a new one to help plan for 1.15

Apologies for hijacking this! I hope the new feature/option is useful for everyone, and that I didn’t cause too much disruption along the way. 😃

Thanks for taking this on, I know for performance it’s nice to back the claim up with hard data, have you ran xhprof or did something point to this as a problem?

I’ve wrote my fair share of micro-optimization patches in my day;)

Ok made a new branch with the new proposal in MR34, though needs a bit of testing, I like this because it won't change expected results from under people who consider root to be the top. Re-titling

joelpittet → changed the visibility of the branch 3321699-setting-use-as to hidden.

Should have closed this, you likely long gone by now. Feel free to open a new ticket if there are reproducible steps

@anybody, I think this would be better as a separate module if you can merge the two. The feature is too substantial to keep as a submodule, so it makes sense to split it out, feel free to take the idea and run with it, let me know if you need something to help the integration, happy to have it as an ecosystem module

Most of what is missing has been addressed. I will close, thanks all for voicing your support for missing features.

Sorry I am closing this, I believe there is an option for this currently as "root", #2950943: Add links to menu parent block title → . Also the MR referenced is for a different issue, please don't cross post @nikhil_110.

I don't think I want to add a route match for the node for the bundle, this can be done in a theme if it's needed, though I can't think of a use-case that would be common enough to warrant the performance hit

@damienmckenna didn't you solve this in 🐛 Empty settings added to all block configurations Fixed . We can re-scope this issue to the clean-up update hook task, if that covers this issue?

@berdir, Thanks for your insight! I ended up just copying the helper method into the test class that was using it, bypassing both the original helper function and the missing helper class altogether. It might come back to bite me later, but that’s a future problem. 😉 If the phpstan on next major causes too much of this, I will do as you say and allow fails, though gotta feel the pain first.

That looks great, thanks for the red/green tests. I scoured over the code and didn’t spot anything concerning.

Darn, I would love help shoring stuff up, feel free to reply Nick if your still game

Re-titling to match https://github.com/Gizra/og/issues/160

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

@nicxvan What should I do when the class doesn't exist, yet like in this case in my test for this change 📌 Move entity_test_create_bundle from entity_test.module Needs work ?

A long shot but could these helpers be committed on D10 or how do we use them in contrib to support D10? I tried copying search_api in 🐛 Fix reliance on deprecated .MODULE.inc files for hooks Active inside og but found D10 didn't have them https://git.drupalcode.org/project/og/-/jobs/4465178

Quick fix, removes all readonly properties so we don't need to worry about 📌 Allow DependencySerializationTrait to work with readonly properties Active

I did my best to merge the diff from https://github.com/Gizra/og/pull/570, there are a bunch of todo's we should fix before merging this, and we should add a way to transition anybody who is currently using og_complex

joelpittet → created an issue.

@dhaval_panara As far as I know, this MR was reverted. It’s highly recommended not to use a GitLab MR URL as a patch because it can be changed at any time, breaking your build—or worse, allowing someone to inject malicious code. A safer approach is to download the diff and apply it as a local patch file.

I’ve seen other contrib modules somehow clean up their config, and I’m curious to learn how this is done.

Just cleaning up, this is indeed complicated and not getting much interest.