Account created on 22 February 2011, about 14 years ago
#

Merge Requests

More

Recent comments

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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)

🇦🇹Austria agoradesign

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 :(

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

@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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

Project page is ok for me.. maybe your browser cache has fooled you?

🇦🇹Austria agoradesign

Well, that depends of course, what is standing in your composer.json - if you have defined a fixed version constraint to a specific release, than of course, you'll have to change it. If you're using something like ~1.0 or ^1.0, you should be fine.

there's one thing I've seen and changed now. the 8.x-1.x branch was not marked as "recommended" I did that now - now the "releases" box on the bottom of the project page is green (not yellow anymore). Maybe this also changes the composer behaviour?!

🇦🇹Austria agoradesign

Maybe he was using a patch at that time, but now this parameter is part of the 6.1.5 release, see https://git.drupalcode.org/project/linkit/-/commit/9412642029f3e5e4d840e...

However, this patch here is outdated

🇦🇹Austria agoradesign

While it's not ideal to have a jungle of different patches and merge requests that cover multiple issues at once, I do not want to hide my solution and share my patch with you

🇦🇹Austria agoradesign

I'll copy here my comment in the Slack thread:

I have seen that there is 📌 Add Capture, Void and Refund / Partial Refund capabilities RTBC adding refund capability. The MR5 of that issue is incorporating further changes into one MR. It handles the API version issue as well ( Make it compatible with latest Saferpay API Active ), and also tries to solve the concurrency issue with the sleep() command ( 🐛 Saferpay webhook faster than customer Active )
Additionally, I needed to allow not collecting billing information from Allow not collecting billing information Active
As I'm not a big fan of the sleep() solution, I've worked toward the solution I have done in commerce_opp, to:
do process the payment in onReturn()
add a dependency on advancedqueue and implement a JobType for processing the payment
only do some basic checks inside onNotify(), but instead processing it, create a queue item and let it handle by cron
additionally, to avoid clashing with a cron run in the same second, I've delayed the processing of the queue item to 60 seconds (which we could make configurable)
I've done different test payments now in staging. If the customer returns, the payment is processed successfully. On cron, the job can early exit, as the order is already paid. It's also easy to close the browser tab after finishing the payment but before the customer has returned to the page. So the order stays as cart, until the queue job has been processed, where the payment is processed and the order placed
So the big question is, if we wanna add a dependency on advancedqueue?! Alternatively, we could do a moduleExists() check and either use the queue or do the sleep (and optionally locking) stuff

🇦🇹Austria agoradesign

Convinced :)

Well, this is one of those modules, that you needed in 1 or 2 customer projects, but isn't used heavily there. You implement it as open-source module, but are just unsure, if it's really that stable that you think it is. Also, you may have some ideas on how to extend it. Provide a block with a nice template for example.

On one project, I've added a block plugin in a custom module for example. This had some project specific touch, which didn't fit 1:1 into the general contrib module, but you hope to find time to find a generic solution for the contrib module...

After some years without complaints, you're right that is time to release 1.0.0

I won't do D11 compatibility check right now. This could be part of a new 2.x branch that uses the semantic versioning syntax (1.x is using the old 8.x-1.x syntax), giving us some theoretical time to add other improvements to that version

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

ok, we actually should have a look at commerce_paypal eg. -> https://git.drupalcode.org/project/commerce_paypal/-/blob/8.x-1.x/src/Ch...

I have to move on now, hopefully finding time on another day

🇦🇹Austria agoradesign

This patch fixes the included promotion rules, but the other ones still fail.. I don't get it :DDD

but maybe it can help in some way...

🇦🇹Austria agoradesign

ATTENTION:

Being under time pressure, I've uploaded my patch, that is hardcoding to restrict the allowed categories to 'pay_later'. Of course, we'd need to define a configuration option for the payment gateway, allowing to optionally restrict the categories, and then doing the filtering.

That's why I'm immediately hiding this patch

However, this is a starting point for anyone coming with the same feature request here, and for finishing the final implementation of this

🇦🇹Austria agoradesign

the last alpha is pointing to the latest commit, so there's currently no difference between them... so I recommend the tagged release

🇦🇹Austria agoradesign

I believe, it could be solved by skipping inside RequestBuilder line 162 not only positive adjustments, but also checking for if $adjustment->isIncluded() -> if it's already included, then we'll need to skip it

I hopefully can find a few minutes to test this theory

🇦🇹Austria agoradesign

Why do you think so?

It's kinda minimally maintained. We still use it on 2 projects currently, where one of them is a quite important one for us. eg if the first one of these sites is upgraded to D11, we'll take care of the necessary changes.

In general, help is always welcome. So, if you want to participate in any way, let me know

🇦🇹Austria agoradesign

agree. this leads me thoughts into the direction that it would be nice to have the possiblity to add some kind of messages to a cart. Like "item XY is not available anymore. Please remove it from your cart..." or maybe then we even could remove it, but show the message to the customer... or like Amazon notify of price changes. afaik, there's already an issue for this

🇦🇹Austria agoradesign

It looks quite random, that the tests fail here and not in general:

Drupal\Tests\commerce_product\Functional\Jsonapi\ProductVariationResourceTest::testIndividual
The 'errors' member was not as expected.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => Array (
-        'detail' => 'title: Title: this field cannot hold more than 1 values.'
+        'detail' => 'sku: The SKU "ABC123" is already in use and must be unique.'
         'source' => Array (
-            'pointer' => '/data/attributes/title'
+            'pointer' => '/data/attributes/sku'
         )
         'status' => '422'
         'title' => 'Unprocessable Content'
     )
+    1 => [...]
 )

That's part of the core's jsonapi base class test testPostIndividual(). Entities with a label key are posted twice, in order to trigger the "field cannot hold more than 1 valoues" error for the title field. As the SKU field has a unique constraint, we also trigger this error. I wonder, why this works for the regular branch, but not for this MR

🇦🇹Austria agoradesign

I also have no idea, why this may happen :( As mentioned, I've already seen this on another site long time ago, when the underlying issue wasn't even committed to Commerce, but we were using that issue's patch.

The fact, that I can only reproduce it on this specific production environment, makes it quite tricky to debug. I haven't spotted any other suspicious code that could mess things up here.

I mainly opened this issue, in order to have this documented and if we're lucky, other people see this too, so that they either find help for themselves or even better someone knows the reason, why the other syntax makes a difference here

For our specific project, there's no budget to invest a lot of time in investigating this problem, as our fix/workaround is patching a single line back to its original syntax

🇦🇹Austria agoradesign

looks good, although I haven't tested it honestly. However, this won't break anything, so I'll merge it

🇦🇹Austria agoradesign

I've added this in the past because of problems with advagg, depending on the configuration (I believe, it appeared when using the JS minification sub-module and maybe depending on how that is configured).

But as this is anyway a workaround on behalf of another module (advagg), which became unusable with Drupal 10.1+, I think it's a good idea to merge your MR. Hopefully, advagg will solve that, if there we'll see a working version again sometime :)

🇦🇹Austria agoradesign

There may be more places in code, so I'll leave the issue open. The last commit also fixes some important typos in the exception messages, so I'll add a release immediately

🇦🇹Austria agoradesign

happened to me in 10.3.0 too, #14 didn't help though.

I had a look at the stack trace, it is related to comment_type's route access checking. admin_toolbar is calling the menu link tree's transform():

    #0 /app/web/core/lib/Drupal/Core/Config/ConfigManager.php(489): Drupal\Core\Entity\EntityDisplayBase->onDependencyRemoval(Array)
#1 /app/web/core/lib/Drupal/Core/Config/ConfigManager.php(352): Drupal\Core\Config\ConfigManager->callOnDependencyRemoval(Object(Drupal\Core\Entity\Entity\EntityViewDisplay), Array, 'config', Array)
#2 /app/web/core/modules/user/src/Form/EntityPermissionsForm.php(96): Drupal\Core\Config\ConfigManager->getConfigEntitiesToChangeOnDependencyRemoval('config', Array)
#3 /app/web/core/modules/user/src/Form/EntityPermissionsForm.php(185): Drupal\user\Form\EntityPermissionsForm->permissionsByProvider()
#4 [internal function]: Drupal\user\Form\EntityPermissionsForm->access(Object(Symfony\Component\Routing\Route), Object(Drupal\Core\Routing\RouteMatch), NULL)
#5 /app/web/core/lib/Drupal/Core/Access/CustomAccessCheck.php(84): call_user_func_array(Array, Array)
#6 [internal function]: Drupal\Core\Access\CustomAccessCheck->access(Object(Symfony\Component\Routing\Route), Object(Drupal\Core\Routing\RouteMatch), Object(Drupal\Core\Session\AccountProxy), NULL)
#7 /app/web/core/lib/Drupal/Core/Access/AccessManager.php(160): call_user_func_array(Array, Array)
#8 /app/web/core/lib/Drupal/Core/Access/AccessManager.php(136): Drupal\Core\Access\AccessManager->performCheck('access_check.cu...', Object(Drupal\Component\Utility\ArgumentsResolver))
#9 /app/web/core/lib/Drupal/Core/Access/AccessManager.php(93): Drupal\Core\Access\AccessManager->check(Object(Drupal\Core\Routing\RouteMatch), Object(Drupal\Core\Session\AccountProxy), NULL, true)
#10 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(218): Drupal\Core\Access\AccessManager->checkNamedRoute('entity.comment_...', Array, Object(Drupal\Core\Session\AccountProxy), true)
#11 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(107): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->menuLinkCheckAccess(Object(Drupal\Core\Menu\MenuLinkDefault))
#12 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(111): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
#13 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(111): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
#14 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(111): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
#15 [internal function]: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
#16 /app/web/core/lib/Drupal/Core/Menu/MenuLinkTree.php(153): call_user_func(Array, Array)
#17 /app/web/modules/contrib/admin_toolbar/src/Render/Element/AdminToolbar.php(47): Drupal\Core\Menu\MenuLinkTree->transform(Array, Array)
#18 [internal function]: Drupal\admin_toolbar\Render\Element\AdminToolbar::preRenderTray(Array)

🐛 Admin Toolbar Tools breaks Dynamic Page Cache on Drupal 10.3 when update module is enabled Active is definitely related imho. A comment there is pointing to 🐛 [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed as the root - looking at the changes made there, compared with the calls in the stack trace, this seems very realistic to me

🇦🇹Austria agoradesign

I have the problem described here 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed since updating to 10.3.0, I saw these lines in the stacktrace:

#17 /app/web/modules/contrib/admin_toolbar/src/Render/Element/AdminToolbar.php(47): Drupal\Core\Menu\MenuLinkTree->transform(Array, Array)
#18 [internal function]: Drupal\admin_toolbar\Render\Element\AdminToolbar::preRenderTray(Array)

I'm sure that these issues are related. Has anyone checked mrstrelan's comment #6?

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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?

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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)

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

looks good, the line in 9.3.3 now is:

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

🇦🇹Austria agoradesign

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

🇦🇹Austria agoradesign

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 :)

Production build 0.71.5 2024