🇦🇹Austria @agoradesign

ok I see. you'd really get an error, if you simply skip the rendering part. I believe, this whole mail sending part must be refactored somehow.

In the short run, it should improve the situation, if the hook_mail() implementation would replace this line:
$message['body'][] = $params['message'];
with $message['body'][] = Markup::create($params['message']);

(of class \Drupal\Core\Render\Markup)

I didn't want to patch around, so this is why I directly tried this approach in a hook_mail_alter() instead:

<?php
function XXX_mail_alter(array &$message): void {
  switch ($message['id']) {
    case 'commerce_stock_notifications_commerce_stock_notifications.notify':
      // Workaround for https://www.drupal.org/project/commerce_stock_notifications/issues/3532501.
      foreach ($message['body'] as $key => $body_part) {
        if (!($body_part instanceof Markup)) {
          $message['body'][$key] = Markup::create($body_part);
        }
      }
      break;
  }
}
?>

The next thing, I'm quite unhappy with, is the commerce_product_variation_url variable for the message template. The story behind is: The message as configured now didn't render with line breaks as it was in the past. So I thought, I go along and override the Twig template directly with my message, as this is the way, I prefer to do it. The intention to use the Url object as variable was good, because theoretically this way you have far more possibilities. But unfortunately I didn't manage to call setAbsolute() followed by toString() in the Twig template, so I've implemented another preprocessor doing this:

<?php
function XXX_preprocess_commerce_stock_notifications_message(array &$variables): void {
  $variation = $variables['commerce_product_variation'];
  $variables['commerce_product_variation_url'] = $variation->toUrl()->setAbsolute()->toString();
}
?>

It is possible to override the template just for this module by using this filename: email-wrap--commerce-stock-notifications.html.twig

However, {{ body|raw }} did not solve it at all for me

The general question is, why you have used the renderer service at all? see https://git.drupalcode.org/project/commerce_stock_notifications/-/blame/...

If you look for example at Drupal Commerce, how the order receipt mail is sent, you'll see that it only creates the render array. Rest should be handled by the mail service:

https://git.drupalcode.org/project/commerce/-/blame/3.x/modules/order/sr...

Is it because of the token handling?

agoradesign → created an issue.

thanks! patched core with the commit from 🐛 Workaround PHP bug with fibers and __get() Active - cannot reproduce the error :)

it's still a one-liner though :D

hmmm maybe it had to do with teh fact, that the user was one with admin rights (having admin toolbar...)

because retried unpatched version with a normal user... couldn't replicate the issue.. but as mentioned in the summary: the ugly version is more resilient

agoradesign → created an issue.

This happened to me on a commerce order page as well, but the customer user entity wasn't deleted at all

Patched 2.2.19 with the commit above, works for me :)

I'll re-open this one :)

On a site, that originally started with Drupal 9 and Photoswipe 3.x, as far as I can remember, we've upgraded last year Photoswipe from version 4.0.1 to 5.0.5

Like mentioned in comment #5, it seems that the update hook is wrong, taking not into account that all these settings are children of "options" key. I can see in my Git commit, that executing photoswipe_update_9003() did not removed any of that mentioned keys that should have been cleared, also it saved the maxZoomLevel wrong: it should have got the value of "maxSpreadZoom", which was also a child of "options" and had the value "2", but in my resulting config I've got then a new "maxZoomLevel" key on top-level with a null value (coming from the update hook's line ->set('maxZoomLevel', $config->get('maxSpreadZoom'))), as well as options.maxZoomLevel with a value of "4", which was obviously set by photoswipe_update_9006() + options.maxSpreadZoom still present

I've also solved it by:

1. Open /admin/config/media/photoswipe in browser and keep it open
2. delete photoswipe.settings.yml
3. run drush cim
4. Saved the admin form
5. run drush cex

the config file was then cleaned up as expected, keeping all the valid existing values

The reason, why I've seen this problem several months after upgrading Photoswipe to 5.x is simple: we've upgraded Drupal from 10.x to 11.x yesterday. D11 is much stricter regarding configuration validation. I didn't see that immediately during upgrade as well, I think, that installing a new module triggered these warnings

--> the more sites are upgrading to D11, which in the past upgraded Photoswipe from 4.x to 5.x, the more reports we'll get here ;-)

I'm sad, that this project is closed down. I absolutely don't like the way Klaro works. I do want a simple cookie banner like this module

important note: if you're still on commerce_stripe 1.x and use MR 25 - beware that this is incompatible with alternative loggers like Monolog, as it includes the problematic code that is addressed in 🐛 Should define LoggerChannelInstance type for $logger, not LoggerChannel Active

you're welcome :-) thanks for sharing your code. I was already worrying about how to solve that

ok, I wouldn't recommend to drop the old table via SQL statement. instead call this Drush command:

drush ev "\Drupal::entityDefinitionUpdateManager()->uninstallFieldStorageDefinition(\Drupal::entityDefinitionUpdateManager()->getFieldStorageDefinition('stripe_paypal_mail', 'commerce_payment_method'));"

Table should be gone afterwards as well, but without complaints on status report page :D

How did you get rid of the mismatched entity/field definition? It says for me that I must uninstall the "PayPal Email" field (the old one)

agoradesign → created an issue.

the MR wasn't merged, seems it isn't mergeable anymore

I just bumped into this, and wonder, why would a 0.0.1 patch revision to Core include this change to ContentEntityStorageBase, shouldn't this be at least a minor revision, i.e. a 0.1? Anyways, thank you #6 for the patch, it's working here as well.

fully agree!! thanks to all helping to get a quick solution here :)

tough question... adding the div is a potential layout-breaking change, that definitely needs to be addressed in the release notes.

If you think, this is the better solution, go ahead and add the wrapper. It can be easily replaced via Twig template override. People just need to be informed about that change

But, if you use the div, I'd rather go for {{ attributes.addClass('review-form-link') }} instead of directly writing class="review-form-link". this way, we're even more flexible. People can change/add attributes in preprocessors, without tpl override

makes sense

no idea. I've never ever used layout builder. So I don't know, if it ever worked

ok, then we have to remove D9 support from 2.x branch

merged, thanks :)

you're missing a pipe character -> it must be ""^1.0 || ^3", not "^1.0 | ^3"

We also see this from time to time.. sadly, this always involves manual correction work

agoradesign → created an issue.

thank you :)

agoradesign → created an issue.

hmmm don't you think that this is just a local dev environment issue, especially as you are using a sub-directory for Drupal installation path? doesn't that break other things too?

that link would make the viewed products more or less uncachable at least...

#3 was talking about "correct values", without giving examples. Have you debugged your site? What values do $referer and $fake_request->getRequestUri() have?

I've tested the functionality. It works for me without problems. What we could discuss, if someone knows a more solid idea to implement this behaviour. Relying on the correct REFERER may not be the most bulletproof solution on earth, as client, server or proxy-server could destroy the logic... maybe the links should use the destination parameter instead. which would mean a more or less bigger refactoring of the module

The core logic simply don't work for these use cases. I'm not 100% sure, if I'm referencing the right core issue, but it sounds good. Take a look at here 🐛 CSRF check always fails for users without a session Needs work

fixed

Is it necessary to re-index the items after updating ai 1.1.4 because of this change?

I've updated ai on 2 sites. in both, RAG queries were failing with error messages like

Could not load the following items on index AI Search: "entity:node/11:de:460601774751947103".

After downgrading to 1.1.3, it worked again on both sites. There weren't too many changes between the two releases, so I expect this change is the one to blame

Sorry for being unclear. I've used my original patch on a 2.x site, which I upgraded today to 3.x - patching failed logically, and I thought that I'd have to rebase it. Unfortunately I checkout out the wrong Commerce branch (3.0.x instead of 3.x), where the related issue wasn't fixed yet. So I've created the patch on the wrong branch and wondered why I couldn't use it with 3.2.0 :-D

long story short: everything's fine, the issue is solved :-)

gonna rebase this

yeah, I've actually seen it.. was wodnering, why patch failed. I was working againg 3.0.x instead of 3.x brtanch :D

just came across this again. I need to upgrade Commerce on that project to 3.x. I don't have investigated this further, but I've updated the patch to be 3.x compatible. At least I keep it in that project for now, and I'll post it here, if anyone else needs it too for 3.x

agoradesign → created an issue.

works for me as well! I've investigated the problem before and came to the same conclusion, that the 'view label' permission should have been used - then found this patch here. Well done :)

daniels____ wrote, that it is recommended, not required.

Although this is or at least was true in former days, I'm against changing this for a simple reason. This change would be an unnecessary and unsustainable step. Drupal 11.2 introduced a lot of OOP hooks, deprecating many of the procedural hooks, we are all used for years. I honestly don't know exactly, whether hook_views_data() does already have an OOP replacement too, but in the long run any hook will be replaced, I guess (comparable with the fact, that any plugin declaration is currently going to be changed from annotations to PHP attributes)

agree that this would be a better solution

wow great, that's super important for Drupal. Thanks to everyone involved! :-)

fyi, just removed this page on a project, where I added it a long time ago, without testing in the meantime, whether it's still needed or not. So, I updated to 7.0.7 and removed the patch. then inserted an image media, linked it, saved. Edited page, saved. edited page, edited the link, saved, and so on... everything worked fine

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

no need to put it in a "hidden" region - just removed the unnecessary function call

idk how much Devel is used nowadays? We stopped using it since we started using Drupal 8 in late 2015.

Anyway, if you commit the code, I'd go for a shorter directory name of the sub module. I'd just use "devel_gen", like Commerce does (cart, checkout, etc)

+1

Then the version of your linkit module would be interesting too, I guess ;-)

It cannot work imho, because there are no configs with "commerce_product_review_overall_rating_stars" in its name.

I believe, the correct way to update field formatter settings, is to first identify the field instances it may concern (we have it as base field on commerce_product entity, but maybe we should search potentially available additional fields in other fieldable entities, as the data model does not restrict its usage (if this was ever used and tested at all, is a different question)).
When we have every concerning field instances, go through all configured entity view display configurations of that entity, and check, if this field component is enabled at all. If so, do the change and save it.

definitely an improvement :)

rhovland → credited agoradesign → .

wow, very quick reaction :)
just wanted to try your patch, when I saw that you've already released a new beta...

Error is gone now :)

opened issue 🐛 "Mismatched entity and/or field definitions" error after 3.0-beta11 update Active

agoradesign → created an issue.

yep, same problem here

please commit and release

thanks japerry, addressed your justified objections and updated the MR. Could you please review?

opened a MR, addressing #4, as I fully agree that CheckoutEvents::COMPLETION is the event that should be used here

And I further have observed that DI is really not working when using a delayed event. I haven't chased this problem down, but I'm 99% sure that the problems derive from serialization and deserialization of the plugin. This is the main difference between the sync and the delayed events

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

Dear maintainer, could you please commit this tiny change and do a new D11 compatible release? It would be really great, as this is only one of 2 blockers of our current project to upgrade it to D11 :)

Would be great to have this tiny change committed and get a new, D11 compatible release :)

I've just started to write a bugfix for this solution, when I saw that there isn't a bug at all, it's just a lack of documentation.

The module ships with a permission log in as another user. When I installed the module, I've interpreted the permission name in a wrong way. So I gave permission to higher (moderator/admin/...) roles only, but in the end it's very important to have this permission on the auto-login user's role. This way you won't get re-logged in immediately, instead may choose to go to the login form instead and login as any user you like or even stay logged out for a while.

Either way, I'll leave this open and tag it with "needs documentation", as this is a very important feature that obviously not everyone finds out immediately :D

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

already did this + other CS fixes, so this MR has become obsolete

merged and released 2.1.0 :-)

Thanks!

no problem :)

Is it incompatible yet?

We are already restricting in composer.json to "drupal/commerce": "^2.29 || 3"

There are no Commerce specific plugins implemented, where changes in the constructor of the base class could cause problems. I don't think, there should be any problems regarding that.

That said, I haven't tried this module yet in 3.x

If you ask rather for D11 compatibility - there's a merge request created by my new co-maintainer @vladimiraus - however it seems, he had to stop working on that (I however guess that there isn't really more work to do on that issue).
I haven't used Commerce yet on D11, because in our setups there are various other contrib modules that weren't D11 compatible yet (at least they weren't the last time I checked)

confirm #136 - patch #135 works until 10.4.1, stopped working with 10.4.2

couldn't apply the MR diff either, so I'm stuck at 10.4.1 for the given project :(

agoradesign → created an issue.

added you as co-maintainer - let's start with "Write to VCS" and "Maintain issues" permissions

I've created a MR with the changes that worked for us in our customer project

agoradesign → created an issue.

nope,doesn't seem so.. here are the currently implemented payment method types: https://git.drupalcode.org/project/commerce_stripe/-/tree/8.x-1.x/src/Pl...

yes, there's 📌 Add bank redirect support (EPS, Sofort) to Payment Element Gateway Active and 📌 Add PayPal support to Payment Element Gateway Active

@jsacksick I haven't really recognized your comment on friday. just saw it :D

hmm well, yes it is specific, but I believe we should be open to custom adjustment types, allowing them to have a solid way to integrate themselves. why should it harm to use events?

We have actually 2 different places inside prepareOrderRequest(), where we deal with an hardcoded opinionated list of adjustment types (skip, discount). my latest commit considers both (I have added in total 2 events, re-using the same event class)

ok, I see, we also need to fix the $breakdown.. but anyway, this can be done inside CheckoutOrderRequestEvent as well, so having this extra event would still be valuable imho

agoradesign → created an issue.

Exactly... I believe, finding a solution for the different browser scenario would automatically fix the first issue, regardless of SameSite cookie settings

@berdir thanks for your input!

That's basically the same code, we also use always. but idk, I have seen some caching issues anyway, after I thought, that the problems were gone. Anyway, I just tried on two of our projects, and I could not reproduce it though. afaik when it happened, it was always the problem, when an anonymous user without a cart/session added the first item to cart

So I honestly don't know, if our current solutions works 100%. At least it does not happen too often, so that I do not get complaints by our clients :D

I'll test this now in a sandbox, report back then

Added 📌 Controllers handling order data should use loadForUpdate() Active and ✨ Klarna Payments Authorization Callback Active as related issues, as both should improve/solve this problem

agoradesign → created an issue.

Happened again... quite every day now.. one thing, I missed out to add: we never have a second payment entity, but checkout complete event is triggered twice, which triggers the order place transition twice, and any event subscriber will trigger twice. So the customer will receive 2 e-mails with two different order numbers, etc

The back button problem seems to happen quite often, at least at the website, we have added Klarna a few weeks ago

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

regarding the wishlist items cache context:

I maybe the wrong person to answer this, as I keep struggling for years to find a way to find the right cache metadata for cart links in the page header, that show the number of items in the cart. Everytime I thought, I solved that, it happened again, that the counter kept staying at 0.

This is little bit different of course. Because every single link depends on the actual items in the wishlist. We may have to think about caching here in general

works for me in test environment. Tried to catch different scenarios (with new created session, with updated session, buy, cancel, select different methods, etc)