🇦🇹Austria @agoradesign

oh that would be great :)

Note, that a manual re-saving of the payment gateway configurations is necessary

The commit contains some refactorings too, but is mainly about this issue

First of all, thank you very much for your MR, Jen :)

It looks 99% good to me, especially the custom event part. One thing I'm not happy with, is the hardcoded and very opinionated CSS selector used for the counter update. eg, this would apply to none of our Commerce sites at all. Would you mind making this selector configurable + empty check for this?

solved as part of #3454250-4: [META] 2.x roadmap →

This is actually a nice-to-have feature in 2.x, probably will be added later

2.x will contain a number of refactorings, causting the single issues not to be separated. A single commit containing all 2.x changes will land soon

ok, we'll actually fetch the JSON from https://docs.prtso.com/brands/json/index.php, ship it with the module and add some needed extra informations manually

agoradesign → created an issue.

After some additional years of implementing/using/debugging/patching Commerce payment gateways, I now see that we have used the payment workflow states in a uncommon way. We were using the authorization state for sole payment intents, where the payment and order process hasn't finished yet. It works very good internally, but upon implementing this task, this is causing a lot of problems for us. So we have first to correct the worklow (use "new" instead auf "authorization" for payment intents, then implement the two different workflows)

agoradesign → created an issue.

agoradesign → created an issue.

agoradesign → created an issue.

agoradesign → created an issue.

I've good and bad news regarding this issue - first the bad one:

The patch does not apply anymore in 10.2.6 due to the changes in 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed

The good news: the patch is not necessary anymore, as the changes in 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed solved this problem in a drive-by :)

Maybe at least another person should confirm that this works now, before we close this issue here

agoradesign → created an issue.

Why should this function return a string? It returned an array before, which makes sense. Now it's a string and the call from the .module file now fails, as you'd now need to decode it again before iterating

looks good, the line in 9.3.3 now is:

if ( isset($action_result['error']) ) {

Great news! :)

Thanks!

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

Same problem as #3 - yes, the url is generated in that form, using commerce_payone

I really hate the whole topic of configuring GTM, especially the consent mode stuff. It should be easy, it looks easy, sometimes it just works, sometimes it's a pita. I never have a good feeling on configuring that. I'm never 100% sure, whether it works like expected.

I (and for sure many many many other people here) would love to see an in-depth guide on how to correctly configure the whole stack - from the Drupal side with google_tag, eu_cookie_compliance and eu_cookie_compliance_gtm, to the GTM side, including GA4, Ads and let's say Facebook Pixel - including steps that other tutorials are often missing, like which triggers to use, etc

eg I stumbled yesterday and today to refactor an older, but small GTM account. The debugger showed me updates of consent states, but in the end, it didn't really matter, whether the storages were denied, because all the tags just kept on firing. I have finally solved it by loading the community template from https://www.simoahava.com/custom-templates/consent-mode/ and also had to build me custom variables, which I then based on the first-party cookie of eu_cookie_compliance, which shouldn't be necessary at all...

So, if anyone has a easy to use recipe for dummies and writes a blog post about this, I'm sure it'll get a lot of views :)

agoradesign → created an issue.

fyi, opened discussion in Slack regarding this: https://drupal.slack.com/archives/C1TLCCF9B/p1709025789941939

Thanks for pointing this out! I wasn't aware of this. imho this is really a huge problem! And I'll raise the priority of this issue, due to the fact that handling payment info is a very sensitive task, and the current implementation might lead to a bunch of problems, and I'll explain why:

First, as described in the issue summary, it's really quite common, that the notification callbacks are called before the customer returns to the site. So, we'll miss a lot of succesful payments due to this. And also as described, you can never rely on desperate workaround like waiting for a second. Because this may help you in let's say 70% of the cases, but sometimes you might need 2 or 3 seconds or even more. so that's never the answer (I have tried similiar workarounds for a different problems with payment notifications on another project)

Second, the main purpose of such webhooks is that a store can receive a reliable feedback on payments. It does happen quite regularly that a user never returns from an offsite payment gateway. Sometimes the user may have closed the browser window too early (some do that deliberately, some by accident). Sometimes the internet connection may hang. Sometimes the Drupal website may have a short down, just in the moment, where the user comes back, or any other problem might occur (eg a database serialization error, etc)

So, it's nice to receive webhooks for status updates, but it is even more important to receive those webhooks to guarantee that we never miss a successful payment, just because the user haven't returned properly to the website!

So, for a payment gateway, that reliably sends webhook notifications, it's far more important to process the payment information in the onNotify() hook than in the onReturn() hook - in other words, it'll better in doubt to skip processing there (only do a short verification of the passed data there) and leave the processing completely to onNotify() than relying on a payment entity created in onReturn()... I can't find the text passage right now, but Stripe even recommends not processing the payment informatin on return, instead rely on the notification callbacks only.

Given my experience with other payment gateways, we might introduce another problem, as soon as we process payment both in onReturn() and onNotify(), because the overlapping requests can cause double-placements and related problems. It's hard to find the perfect solution for this problem then. I've tried different approaches, which all work quite good for us. The latest one was to process payment info in onReturn() directly, and in onNotify() create an advancedqueue job item, which will be processed on cron then. This way, you get rid of 99% of conurrent request problems, can send an immediate response to the payment gateway, which is also important. And in most of the cases, there's no need to do any processing anymore, as the order is already placed in Drupal, when cron runs. But for the small amount of use cases, where the user never returned, the order is placed on cron then

Coming from 📌 Add PayPal support to Payment Element Gateway Active , the outdated PHP library is at least a small problem for me.

I started implementing the PayPal payment method plugin - thought it might be a good idea to store the PayPal e-mail address in a field, so that at least we can use it for building the payment method's label. In the currently used old PHP API (7.x), \Stripe\PaymentMethod class does not contain a StripeObject representing PayPal data, but the current version does have one (was added about 9 months ago)

I'm gonna try to pick the e-mail address from the billing details instead for now, but I may encounter the next problems, when I try to implement another plugin... so I'm totatlly in favour of upgrading the PHP library

PS: also, what about PHP compatibility? Is it fully PHP 8.2 compatible? If yes, that's random good luck, but we may get problems in future versions... so let's start upgrading :D

You'll need this ✨ Add the option to not setup payment methods for future usage Needs review

I know. and as I have written, the mentioned commit seems to have the drawback that currently only credit cards work, others need their own plugins now

I haven't tried them yet, but I'm wondering.. which version do you use? 1.1? because in the meantime (dev, 1.2-rc1) this commit may have broken these payment methods: https://git.drupalcode.org/project/commerce_stripe/-/commit/bf118321d3fb...

agoradesign → created an issue.

agoradesign → created an issue.

Side notice on this issue: I'm coming from 📌 In StripePaymentElement->onReturn do not allow PaymentGatewayException to bubble up if we know the payment was successful on Stripe's end Active because I've realized that any other payment methods than credit cards (and the American ACH banking) are currently not supported by Payment Element, like in my case PayPal was refused on the Drupal side (the Stripe transaction was successful)

I found this issue description here a little bit weird at first, as it clearly states that PayPal payments already have worked at the time of writing this issue. This important refactoring here destroyed PayPal and other payments until a dedicated plugin is created: https://git.drupalcode.org/project/commerce_stripe/-/commit/bf118321d3fb...

I'm already on latest dev/rc. You're right that a necessary refactoring work has already been committed, which paves the way to implement other payment method types, but nevertheless they need to be implemented like 📌 Add ACH support to Payment Element Gateway Fixed recently.

Ok, you're right. I'll open up issues per payment method type

thanks for your input!

I think, the best would be to implement the missing plugins. That shouldn't be that tough imho. I'm of course willing to help.

I just want to coordinate to avoid working in the wrong direction. So basically, here are the plugins we'll want to use:

• PayPal
• EPS (Austrian bank redirect)
• Sofort (bank redirect in AT, DE, CH)
• Google Pay
• Link

The last two are optional for us. The most important will be PayPal.

We should coordinate, which extra fields we want/need, any special treatments (like status processing) that are needed, and so on. I think that there could be either a generic plugin for bank redirects or at least an abstract base class to hold common logic together (rather this I think). Any important thoughts, links to important parts of the Stripe API docs, etc would be very helpful.

As next step, I'd open issues for any plugin that I can help to create. But let's discuss the general approach either here or in Slack first

I'm coming from 💬 Support more payment method types Closed: duplicate . Our client reported a problem in his webshop. A customer paid successfully with PayPal, but got an error message on Drupal side, because PayPal is currently not supported by this module and a PaymentGatewayException is raised with this message "The selected stripe payment method type(stripe_paypal) is not currently supported"

We were not aware that not every activated payment method in Stripe is allowed on Drupal side - and now I'm in a hurry to get a quick workaround. We don't care about re-usable payments here, so having a payment method entity is not important for us. So I'm just thinking about patching the onReturn() function to early exit instead of throwing that exception. --> would that probably lead to other problems inside commerce_stripe module? Because if not, I'll do that as a quick fix. Any feedback is very welcome :) (also on the commerce slack channel). I'm of course open to help implementing outstanding payment providers

agoradesign → created an issue.

@nchin - I've committed the patch from #2

So you can try the latest dev release without patching.

Although the change is tiny and looks 100% logical and correct, I haven't actually tested the change. So I think, I'll wait with a tagged release

Why did you use a leftJoin() @Rhezios? Leaving join() makes more sense imho

oh yes, you're right. didn't know that too... ok, then everything's fine now :)

adding a PS to #33 - it is both totally unrealistic and imho semantically totally wrong refactoring the architecture in branch 2.x to allow kind of dynamic properties in a future-safe way. So please FMB, feel free to bring in your ideas in a separate issue either for version 3.x or maybe a future 4.x branch, and anyone here will really appreciate your efforts, but as Damien already stated, this issue is about making the module PHP 8.2 compatible, so that we are not forced to switch to 3.x immediately, which caused me some problems a while ago. And the proposed patch does exactly that. It adds some properties, and doing this does not make anything worse regarding class inheritance.

imho the only thing that is discussable in that matter, that we should think about not only adding the two class properties as proposed, but also add the AllowDynamicProperties PHP attribute, so that any custom extensions are safe regarding this matter in PHP 8.2

Either way we decide, this is RTBC and will hopefully land soon :)

agree, please commit!

although 9.5.x is not supported anymore, here's a re-roll for 9.5.11

It's inside an or condition

Coming from #3403745-4: Why is stripe-php still stuck on version 7 and Stripe API on 2019-12-03? What does it imply? → , I've tried this patch (latest MR) and it solved our problem :-)

I can confirm that this solved our problem :-) thank you so much!

This is indeed very likely solving our problems :-))) I'll have to wait until afternoon to enroll it to production environment, but it looks great in dev

@skyredwang thank you very much! I'll try tomorrow

yes, it actually was so :( but you're right - better safe than sorry :)

I'll have a look for sure - if it will be re-added to IEF again, it'll be present in any of my projects too :D

PS: aaaaaaaaaaaaaaa I'm sorry, I've just read trough the related issue 💬 Dependency on RenderArrayTool Active , and now know that the rat module is more a library than a real Drupal module, so it would technically work, without enabling the module, I guess. However, this would be a very unconventional approach for a Drupal module dependency.

I have done manual code review before updating to rc18, instead of just tryin' - and that new dependency, that wasn't even declared in info file, nor installed in update hook, simply refrained me from even trying to update.

So my conclusion: rc18 wasn't that "broken" - or better BC-breaking - as I thought, but I don't really like the approach of adding a Drupal module dependency without needing to install it - so I highly appreciate the revert in rc19 :-)

You should add this dependency to the info file as well, otherwise it won't get automatically installed, when you enable IEF. imho we'd also need an update hook, checking if rat is already installed, otherwise enable it

ok thanks anyway! I have already contacted Stripe support. Via chat, they couldn't help me, because everything looks fine at first glance. So I'm waiting for an e-mail after they have further investigated the problem

Nicolas, do you see all enabled payment methods? I'm currently stepping in the dark, why only credit cards and Google Pay is shown, although several others are activated. I am not seeing clear signs, why the others should not be visible. I have also seen now in Stripe Admin that there are a couple of old API calls (2019-12-03), which brought me to this issue here.

Do you think, the old API usage is the reason?

agoradesign → created an issue.

Well, it seems that those changes in 10.1 have fully changed the way CSS and JS is aggregated, so that at least the current state of advagg is useless - i don't know, if there are possible improvements, where a refactored advagg could hook in and help

What I miss most, are some of the sub module features. We were using the defer CSS a lot in newer projects, and I had to drop this unfortunately because I followed the advice to not use advagg in 10.1.x

@Dom. so you are using advagg with 10.1? I needed to uninstall advagg as a workaround for my 10.1 sites :( see 📌 Document which parts of the module are still relevant after aggregation changes in 10.1.0 Needs review

This should be the only way to apply patches :) https://medium.com/appseed-io/apply-drupal-9-patches-with-composer-7b1e5...

haven't tried it, but checked the changes. That will fix it for sure, thanks!

Are you sure? Have a look at this one: https://git.drupalcode.org/project/google_tag/-/blob/2.0.x/src/EventSubs...

regarding #24

As suggested in #22 and #23, disabling the cache on the media library widget view (/admin/structure/views/view/media_library/edit/widget), seems to resolve the issue (we've only disabled it on the grid widget, not the other displays)

After saving the view configuration, I've observed one thing. I don't know, if this makes any difference. I also only unset caching for the "widget" display, but the config for the other displays slightly changed too. The previously empty "tags" setting got filled with dependencies to different media EVD configurations. I do not believe that this has anything to do with the problem here, but I may be wrong... this part has changed (of course it'll differ from site to site, depending on what media bundles and view modes you have)

      tags:
        - 'config:core.entity_view_display.media.document.default'
        - 'config:core.entity_view_display.media.document.media_library'
        - 'config:core.entity_view_display.media.image.banner'
        - 'config:core.entity_view_display.media.image.default'
        - 'config:core.entity_view_display.media.image.gallery'
        - 'config:core.entity_view_display.media.image.hero'
        - 'config:core.entity_view_display.media.image.media_library'
        - 'config:core.entity_view_display.media.remote_video.default'
        - 'config:core.entity_view_display.media.remote_video.media_library'
        - 'config:core.entity_view_display.media.video.banner'
        - 'config:core.entity_view_display.media.video.default'
        - 'config:core.entity_view_display.media.video.hero'
        - 'config:core.entity_view_display.media.video.media_library'

I'll post the "cron_data" part (because this should be the concerning part) and leave out the sent key:

    "cron_data": {
        "cleantalk_cron": {
            "sfw_update": {
                "handler": "\\Drupal\\cleantalk\\CleantalkFuncs::apbct_sfw_update",
                "next_call": 1699951893,
                "period": 86400,
                "params": []
            },
            "sfw_send_logs": {
                "handler": "\\Drupal\\cleantalk\\CleantalkFuncs::apbct_sfw_send_logs",
                "next_call": 1699955433,
                "period": 3600,
                "params": []
            },
            "sfw_ac__clear_table": {
                "handler": "\\Drupal\\cleantalk\\CleantalkFuncs::apbct_sfw_ac__clear_table",
                "next_call": 1699955433,
                "period": 3600,
                "params": []
            },
            "sfw_update_checker": {
                "handler": "\\Cleantalk\\Common\\Firewall\\FirewallUpdater::apbctSfwUpdateChecker",
                "next_call": 1700124146,
                "period": 15,
                "params": [
                    "***APIKEY***"
                ]
            }
        },
        "cleantalk_cron_last_start": 1700124089
    },

no "processing" key in there - I guess this is only checked inside code, and as far as I see, it is only set to TRUE under certain circumstances, but never initialized with FALSE. But there is a check against this value, and that's why we get the warning

What kind of sensitive information is inside these logs? I see that the API key is in there - anything else I should remove before posting it?

btw, I do have one question: these classes within the "lib" directory - is this a general PHP library of CleanTalk? And if yes, wouldn't it be better for both your devs and us users, if load this as Composer dependency (in version 10.x)?

agoradesign → created an issue.

already done by another user, see 🐛 Undefined array key "processing" in Cleantalk\Common\Cron\Cron->checkTasks Active

no, not all of them. I was already using 9.3.0 and still had one of these deprecation warnings:

Deprecated function: Creation of dynamic property Cleantalk\Custom\Db\Db::$db_result is deprecated in Cleantalk\Custom\Db\Db->fetchAll() (Zeile 81 in /web/modules/contrib/cleantalk/lib/Cleantalk/Custom/Db/Db.php)

With the given patch from #2, there's the risk of running into an error:

TypeError: array_merge(): Argument #2 must be of type array, null given in array_merge() (line 191

This seems to happen, when the user does not have the 'use menu link attributes' permission set

I've updated the patch to ensure an empty array, if the value is not set

I have the same problem, but with undefined array key "processing"

I've already seen it with 9.2.7 on Drupal 9.5. Now, I have updated the site to 10.1.6, updated cleantalk to 9.3.0 + uninstalled and reinstalled the module - the log messages still appear on cron run

+1 for #12

Thanks Ryan! please keep us updated

agoradesign → created an issue.

Module is obsolete

yeah! thank you!!

thank you! that's better than nothing, but nevertheless it would be important, that the maintainer of this project could commit this into dev

btw, as I see that you're doing a bunch of code style fixes in your MR: you've missed out that single and redundant ";" in ConfirmClearSubscribers (you have moved that into a single line)

works for me (Drupal 10.1.5 + Gin Admin Theme)

D10 compatibility would be important

changed priority to critical, now that Drupal 9 is reaching EOL next month. there's not too much to do here.

I know, we all spend our spare times for maintaining our Drupal modules, but please take these few minutes and make this module D10 compatible

I think, this should not done before releasing a new version

Are you a real dev or a bot? If dev, have you even read your own patch and the underlying code? There's only one helpful change in your proposed patch

Such things may happen... version_compare() can be quite confusing to read

you're welcome... yeah, I know - but I felt it's ok to leave a comment here about this tiny problem. at least it was caused by this issue's commit :D

After updating from beta1 to rc1 I see errors on the status report page:

"Photoswipe v5 currently only supports photoswipe library version minimum: 5.2.1
You need to install a compatible version!"

Indeed I've the latest Photoswipe 5.4.1 (and $library_version is indeed "5.4.1")

On looking at the latest commit (https://git.drupalcode.org/project/photoswipe/-/commit/99ea723a66fc90e10...), I see that the version_compare() was changed, from:

if (version_compare($library_version, $min_req_version, '<')) {

to

if (version_compare($library_version, $min_req_version) >= 0) {

And this checks the opposite of what we want. see example 1 here: https://www.php.net/manual/en/function.version-compare.php

Proposed resolution

Revert the concerning line to

if (version_compare($library_version, $min_req_version, '<')) {

here's the link: https://www.drupal.org/project/symfony_mailer_lite →

regarding Core discussion: it's a shame imho that we need a contrib module anyway. HTML e-mails and configuring the mail transport (SMTP or whatever) must be part of core normally. Would be great to see stage 1 coming asap, leaving enough flexibilty for contrib modules like this one to add extra features like defining policies

I'd like to add my 2 cents to this discussion as well, before closing the issue

A couple of days ago I've read in the Commerce slack channel about a more simple alternative to symfony_mailer, offering exactly the same basic functionality as swiftmailer. The maintainer of that module also had some pain points with this module, so decided to offer an alternative implementation. Though I personally haven't tried that and I'm not planning to do so - because for the simple reason that I haven't experienced any problems so far :)

We all know situations that you can run into problems with modules that many people use and all the other seem to not have these problems at all. Sometimes, your specific project or your general specific workflow may lead to compatibility problems with other code parts. In this situation, you may feel that module/plugin XYZ is bad and to blame to occassionally destroy your site. But that doesn't mean that most of the other users feel the same.

I must confess that I've waited quite a long time to switch from swiftmailer to symfony_mailer - I waited until I finally created or upgraded the first D10 site - because in the early days of symfony_mailer module all the new concepts just felt to heavy-weight. there wasn't a good guidance at that point, and the whole mail-sending part of a website is normally for me a very unimportant side-feature, which just has to work.

When I first used a stable version of symfony_mailer, I was surprised in a very positive way - the upgrade experience and the UI was great. Although I haven't really tried advanced use cases with specific configurations, I'm very happy about that the module can be installed easily and just does its job. It's also easy to switch the configuration in dev environments (eg provide a mailhog transport policy and enable this via local settings). It's far more mature imho than swiftmailer ever was

Finally, I want to add, that I am always relieved when I see that @AdamPS is the maintainer of a project I'll want to use. Because then I'm always very confident that the code quality is high. So thanks for your hard work on this module and any other project you maintain :-)

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

I'll go along with that :-)