🇩🇪Germany @Anybody

Duplicate of 📌 Fix phpstan Needs work ?

Merge conflicts! :/

@awolfey could you please provide this as MR?

@W01F would be great if you could provide more details!

Yeah this should work out of the box and totally make sense. I'd rate this as bug, could we start with tests showing that it doesn't work as expected yet and improve the issue summary to tell what's expected and the actual state?

Or is this simply a duplicate of 🐛 Still not working with certain letters Needs work for e.g. Japanese characters?

Would this improve the situation maybe? ✨ Support tippy.js by submodule Active

@W01F & @jacobupal

This would be fantastic as a submodule... just saying =)

Yes I think that would be the most robust and simple solution.
glossify_tippyjs or something like that. Is tippy the best choice or maybe popper or something else?

https://www.reddit.com/r/reactjs/comments/pi7cu5/popperjs_vs_tippyjs_whi...

The submodule could then also act as boilerplate for other tooltip libraries.

Happy to review MR's! :)

I guess this might be closed as duplicate of 🐛 Still not working with certain letters Needs work maybe? Also should be against 3.x now.

Thank you @sanduhrs, great work! Could someone please add a test for these cases?
Do we need other tests due to the modifier addition?

https://www.php.net/manual/en/reference.pcre.pattern.modifiers.php

@draenen could you please prepare a MR from the patch? And maybe it would be worth to link this issue in the comment?

Are there better alternatives to this approach, maybe?

This should be fixed in the 3.x version.

Thank you very much for the implementation, folks!

Now I'm wondering if we should really solve that on our own or better use a more accessibiliy friendly tooltip library? What do you think and is one out there?

Hi and thank you for the report. It's not yet clear to me, what needs to happen here and how to reproduce this. Could you update the issue summary with clear steps please?

So is this similar to: 🐛 Twig debug mode causes parse error in appendXML Needs review just with another effect?
Or is an implementation of https://twig.symfony.com/doc/3.x/filters/spaceless.html needed in the twig file?

Happy to review fixes as MR.

Someone should please test, if further code changes are required.

@ressa thank you very much for the idea. I first thought, what you describe should better be a separate module or a submodule, but essentially your implementation just adds a further replacement.

Now I think it would be even better to implement token support here to unify the functionality instead of adding further custom syntax / pseudo-tokens. What do you think about that?
Could you do it?

@Andrés Chandía so you're suggesting something like a "ignore list"?

Feel free to create a MR with that feature or pay a developer to do so. Thanks for the suggestion!

I'll tag a new release with this fix!

Great work, thank you both!! Merging!

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

Anybody → created an issue.

Made the title reflect the implementation.

The fork needs to be updated and conflicts needs to be resolved. Anyway, this looks quite far implemented. Thank you @tBKoT for pushing things forward.
✨ Implement a generic revision UI Fixed and 🐛 Revert and Delete actions should not be available for the current revision Fixed are both resolved in core. So this should be compared to what https://www.drupal.org/node/3160443 → suggests.

Maybe "Needs tests" tag can be removed, I can see the Mr has tests.

I think there are quite a few projects where it would be great to have revisions for products and product variations to be able to track changes and revert them if needed!

Good enough, thank you :)

Here are some possibly related core issues, maybe someone can work through them in the future:

General search: https://www.drupal.org/project/issues/drupal?text=rebuildContainer →

• #2066993: Use \Drupal consistently in tests →
• #2252967: rebuildContainer() is not limited to web tests (WebTestBase) →

Reading through the comments in #2066993: Use \Drupal consistently in tests → and the blog entry linked above, $this->contianer might be the root of all evil and this issue: #2066993: Use \Drupal consistently in tests → might be the core issue to solve it?

I agree, we should try to get rid of these unclear $this->rebuildContainer(); calls that seem to be kind of a Drupal Core bug.

Thank you for the documentation. This isn't important and if we find a general core issue, let's postpone this.

To see if this has been fixed upstream already, let's simply remove these calls in the MR here! :) So if the test goes green, it finally works correctly without these calls.

@LRWebks thanks, I left some final comments. We could have some more tests to be even more sure it works as expected:

• Remove all notification object entries (this should also have a mail test as we have the new case that the notifications objects can be empty in config now)
• Remove multiple notification objects at once

Could you add these finally? Should be simple, I think.

Anybody → created an issue.

For the different display options: To open the webform in the modal, the relevant webform global option must be enabled:
https://www.drupal.org/node/2972533 →

We should link that information and eventually check the value of that config variable?

@amityweb please don't complain, but take the time or invest money to finish this. This is open source, it's not someones job ;)
I understand the frustration and I'm running into the same sometimes, but it's not fair.

What have YOU done to resolve this? (Looking above the answer seems to be: "Nothing", but hopefully "yet" ;))

And yes, I agree this already took a lot too long, but comments like that don't help getting things done, please remember this. Participation helps!

GREAT JOB @LRWebks! :) 🎉🎉🎉🚀🚀

Great, thank you @jsacksick! Nice to have this new, important feature in action :)

Anybody → created an issue.

Anybody → created an issue.

Update: This is now the last patch required to make commerce_api fully work: 📌 Document recommended patches Active

Still NW as of #5, but close to the finish line.

🐛 Support non-entity bundle types Fixed is now fixed!

I'll remove it from the MR. ✨ Support for cross bundle resource types Needs work is the last one now to be fixed for commerce_api! 🎉

Anybody → created an issue.

Nice @LRWebks I left some final comments, afterwards this LGTM and we should tag a new release after merge.

Anybody → created an issue.

Seems currently version 4.x is expected?

Drupal 7 expects the following:

if (file_exists($dir . '/iframe-resizer/js/iframeResizer.contentWindow.js') && file_exists($dir . '/iframe-resizer/js/iframeResizer.js')) {
      return $dir . '/iframe-resizer';
    }

Also it's not totally clear which library path structure is expected.

Anybody → created an issue.

Thanks @rp7 - could you turn this into a MR please?

Thanks for the confirmation @bdunphy, then let's make this major

Thank you @greggles!! :)

@smustgrave FYI:

instance.options.data.dialogOptions.settingsTrayActiveEditableId = closestSettingsTray?.id;

is the shorthand for

instance.options.data.dialogOptions.settingsTrayActiveEditableId = closestSettingsTray ? closestSettingsTray.id : undefined;

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operat...
Browser compatibility is also fine.

Of course I'm not sure if the proposed resolution is acceptable at all, as it doesn't really fix the root cause.

Thanks for the feedback @drunken monkey - this also happened on cron run. Really really strange thing and wild side effect I think.

This didn't happen again now, so let's for now close this cannot reproduce and if someone runs into the same issue, let's reopen this. I guess it's a very very specific case and not really the fault of this module.

Going to tag -rc6 in some days, waiting for further feedback here.

Merging this into 2.x-dev now based on the feedback, thank you!

Re #4 okay I'm fine with that!

Great work! Let's merge this and see if both (now failing here) PHPUnit tests then work in the related issue.

Okay I was now able to reproduce this issue without this module in Drupal core, by submitting the exposed filter on the views page directly. So I created a Drupal Core issue: 🐛 Views exposed form doesn't handle non-array values correctly, where an array is expected ("Allow multiple selections") Active

I guess it needs to be solved upstream. Any ideas?

Anybody → created an issue.

@Grevil: Please review by time.

Anybody → created an issue.

Thanks @boquistm.
The problem is, that we can't do anything about these URL values, as they are request parameters. So we'd need the views exposed form to filter them and only handle the ones we expect.

I think this is the line where the errors starts:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Maybe we'd need to use "setProgrammed()" on FormState, but I'm still quite unsure about that. I'll add a MR doing that and I already tried: It removes the issue and the exposed filter block still works as expected in my case. Still it may have unwanted side effects.

We'll need to investigate further, if that's the right way.

Furthermore, I'm not even sure if we should really always process the exposed form when just displaying it. It may depend on the case used and could be a configuration option?

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

Confirming that. MR!43 LGTM, thanks!!

Nothing to do here @Hetal.Solanki. Please don't touch.

Anybody → created an issue.

Thanks, yes totally with you! :)
We just need a direction to go, so we don't waste valuable time. So let's wait for your and your teams feedback! Thanks!!

Thanks @jsacksick yes it should definitely be backwards-compatible.

This is relatively easy to fix, by altering the route and specifying a different title callback. I'm guessing for some merchants, it might actually make sense to output the SKU in there too?

Would you accept this in Commerce Core or does it have to be solved in contrib?

For the other points, can you tell which of the proposed changes from the IS you would accept for Commerce 2 and which could be considered for Commerce 3 future?

We'd then create a META issue, split the proposed points into subissues and start working on them where it makes sense based on your decision.

In general my late hope is that one day you'll perhaps see the points yourself, at least partially, so we can revive it ;) I don't think I'll be able to convince you that most of this makes a lot of sense, if you don't run into this yourself :P
And yes, Commerce 3 would be fine for the large parts, as I think this is a conceptual thing. And of course especially for breaking changes!

@jsacksick thanks! The issue summary already points out the things we experienced very well.

The final thing that lead me here, is the slight visibility of the parent product in the product variation CRUD forms. (e.g. /product/43/variations/9/edit)
Product variations have a very strong relation to their parent products (can not exist without), but working on such a form you feel to be "somewhere".

The same happens to us in other places where product variations are shown, especially in the admin backend. As a workaround you can prefix all product variations with the product name, but that leads to other issues.

So all in all we came to the same conclusions suggested in the issue summary to be able to bring the product variations closer to their parent product con(textually) all over the projects.

And yes of course there are workarounds, but it seems to us that this is a conceptual thing that should be improved in general and we're willing to help, as it would help us and our clients.

Bumping this major issue.

Well done, thank you! Let's merge this :)

Final improvements (documentation). Then this is ready! You've tested it manually?
We don't need a test for this.

Re #3:

• We have to put our logic in front of sending these emails, so all the logic is inside our module, before passing them to the mail() method. To be more precise, I see two methods:
  • Frequency capping (stop mailing after sending X notifications already)
  • Delayed sending (do not send emails for a certain amount of time and then send a digest)
• Yes [Limit] per [Timeframe] is correct. We're only looking at our (not yet sent) log mails.

As log notifications are time-critical, I think we need to use frequency capping, so we should stop sending further notifications if we sent X mails per timeframe already. A bonus would be to not send the same email too frequently, because it might not be as helpful to see the same message a thousand times, but to see different messages instead.

See https://github.com/d6lts/drupal/pull/79 for Drupal 6 LTS if you're running into

AH: Unsafe URL with %3f URL rewritten without UnsafeAllow3F, referer: ...

Apache errors after Apache updates: https://support.plesk.com/hc/en-us/articles/13302819141783-Domain-in-Ple...

I'll tag a new release!

Yeah clear mistake on my side.

Anybody → created an issue.

Great! :)

Ok @Grevil take a look then please, not urgently.

@ptmkenny could you do the same for 3.x?