🇮🇳India @sorabh.v6

Starting to work on this issue.

I will again try to reproduce it and if its still an issue then we can re-open it maybe. But I do not see any other person reporting the issue so maybe its fixed or maybe its just me who getting this issue.

Updated the code in the template file and its showing up now. Please review.

There's some existing code that I think is not useful and we can create a new issue to remove the unnecessary code.

Thanks

Working on it.

MR created. Please review.

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

I have added code that checks if assets is a valid scheme or not to the DrupalFilesSourcePlugin.php class but I was not sure how do we achieve this for the update hook.

Update hook ran successfully when I switched from 5.0.3 to the issue branch.

Hi @jdleonard,

I attempted to fix the addition of default config for exclude_filepaths (which appeared to have a copy-paste typo and wasn't analogous for the three sources) though please double check that I didn't misinterpret the intent / behavior

Thanks for spotting the typo.

Is exclude_private_filepaths (for example), correctly named? It's not clear to me under what circumstances it would ever be set and thus why it was added.

It was code refactor residue. I will remove it. Thanks for pointing it out.

I haven't been able to figure out why.

I will try to check.

Code pushed to the MR. Please review.

Thanks

Updated the issue summary's proposed solution section.

@berdir I think this is not an issue with just summary. I added a field with machine name url and put that in the description meta tag for testing and in the description meta tag its showing node url instead of the value in url field.

Therefore, I think we should work on this issue.

sorabh.v6 → changed the visibility of the branch 3520386-option-to-exclude-2 to hidden.

Working on it.

I double the idea, adding a note field or a similar field will later help to cleanup the redirects if they are not needed any further.

MR created. Please review.

Working on this issue.

In my opinion the time slot can be added an another feature for the module because not many shipping providers around the world provides the time slot facility for the shipment. They just provide the estimated delivery date. So, maybe we can work on the estimated delivery date in the issue and create another one for the timeslot with the settings to enable this feature.

I have created the MR. I worked on it for so much time. Initially, I thought I could just add the validation and then I see that element validations are different than form validation itself and event that didn't worked because there was not identifier to know if the element weight is changed.

The better and I think only approach to solve this was doing some checking on the frontend side using the JS.

I am adding the warning message when an element is dragged to save the form before making any changes. Even if user ignores that warning and just click on delete action link for the element then I disabled the delete button and show the same message as error in the dialog modal.

@smustgrave I have removed the line in the feedback and pushed the code.

Thanks for the feedback.

@smustgrave thanks, I will look at the review comments and work on them.

PR is ready for review and I am ready for feedback. :)

I was not able to reproduce it. Though I faced another issue. The title of the node is "This is it. Now I will focus" and it generated the url alias as /it.-now-i-will-focus

See the "This is " is removed from the alias.

If someone else also sees this pattern then we can create a bug issue for this.

Else, you can see that period is in the url alias and therefore this issue is not reproduced.

Will try to reproduce and work on it.

I think the proposed solution is logical but # is also hash for some. How do we differentiate and is there a list for such symbols?

Starting to work on this issue.

Tests failed. Assigning back to me.

Working on this issue.

Just for FYI, I looked into the issue.

I created the node and then translated it.
Then I commented on the english translation. It is showing fine on the english translation.
Then I viewed the french translation and commented on the french translation. However, the comment on the french translation is attaching to the english translation. I think I will need to correct the comment to parent entity mapping based on the parent entity's language.

Continuing on the issue, so far no success but I will get it there. :)

Starting to work on this issue.

This isn't needed with "display_description", correct?

Yes, but we can show the description.

Starting to work on this issue.

@jsacksick

Also, let's update the promotion order processor test to test the case where multiple coupons are allowed using the new setting (we're missing that).

Do you mean this code -
https://git.drupalcode.org/project/commerce/-/merge_requests/407/diffs#d...

All the child issues of this task are up for review. Please review them and provide feedback and if all's good then the maintainers can merge the code and close these issues.

Working on this, I solved a few phpcs issues already. There have been some deprecated warning. The ones that are drupal related like hook_hook_info and hook_hook_info_alter, I have corrected them. Others, I tried to find the CRs and any useful information but could not and therefore I removed those deprecated text.

Code from #28 pushed to the MR and also fixed the test.

Thanks for your reply. Maintainers of the module will be able to merge it.

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

Pushed the code from #58 to MR and tested on local. It worked and even passed all the tests.

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

Thanks and please post your reviews here. Help us close this issue. :)

@charles belov I went through the steps and I am able to reproduce the issue. But it is working correctly. I will try to explain.

[current-page:title] token gives you the title of current page. And the title of node add form is Create Basic Page and therefore the pathauto is creating the alias as /create-basic-page.

The method that you are stating as workaround is actually the correct way of creating alias using the node title.

For more clarity refer this code https://git.drupalcode.org/project/token/-/blob/8.x-1.x/token.tokens.inc...

Marking this issue as works as designed and unassigning myself.

Thanks

Starting to work on the issue.

Working on it.

Hey @jsacksick, I have made the changes. Let me know if you think anymore changes are needed.

Hey @jsacksick, I will move my changes to a new method in the CouponTest. I hope that would make it better to review.

Code pushed to the MR, moving task to "Needs review".

Working on this issue.

Working on it.

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

I think its doable.

As per the conversation in the slack -> https://drupal.slack.com/archives/C1TLCCF9B/p1740661087476619

Updated the MR and needs review now.

Thanks

Code pushed and MR created. Needs review and if all's good then we can merge the code and close this issue.

Somehow, I was not able to reproduce the issue even when tried multiple times. Please look at the uploaded video.

@jeffam please confirm if I am following the right approach to reproduce the issue. If yes, then I think this issue is resolved as part of some other issue and we can close this issue.

Thanks

Actually, when multiple coupons are not allow and we apply coupons from UI then we get validation error. But when we apply the coupons programmatically then we do not get error and multiple coupons are applied.

I thought to prevent this I need to update the code in order presave.

If you do not want me to update the code in the order module, then I can add that validation in the hook_entity_presave.

Let me know if that works.

Thanks

@jsacksick There's some issue with the phpunit pipeline. Its giving me failed tests for the files which I did not changed.

Other than that, MR has the updated code that has -

• need a description explaining what the setting does
• PhpUnit tests for the change

Let me know if I understood it wrong and those failing tests are related to my code.

Thanks

Sure, I will add test coverage for this extra setting. I will add description/help text for the field as well.

This works fine for me.... did a `drush pmu commerce_product && drush en commerce_product -y`. It just worked and I am not getting any WSOD now.

Hi, please review the MR https://git.drupalcode.org/project/commerce/-/merge_requests/407

I think its better that we add this into the 3.0.x-dev.

@jsacksick Let me know you opinion.

I am interested in working on this issue. Just to confirm, we are going with "usage_limit_order"? I was thinking this name might be little bit confusing.

IMO if we put a checkbox in the usage limit details container that says "allow applying multiple coupons per promotion". If this is not checked then we can compare promotion id of the applied coupons else we compare the coupons id and promotion id.

Let me know how this sounds to you @jsacksick.

Thanks

@mbopp I think this task is related to this one https://www.drupal.org/project/commerce/issues/3005944 ✨ Order items should store the SKU of the purchased entity Active

Working on this.

sorabh.v6 → created an issue.

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

@Dmitriy.trt Since D10 is already released, I think we can close this issue now.

@Wim Leers I am not sure now how to properly provide the steps that would help to reproduce the issue. Can you tell me what more details is needed and how I can provide those details. I mean, how do I check this -

It's possible that the server side did create/start a session and infrastructure between client and origin prevented that session cookie from reaching the client.

Or is it drupal commerce related issue?

Hi Wim Leers,

We are experiencing the same problem on a Drupal 10 site. This site is using the commerce module.

Visit this demo site.
https://commerce.demo.centarro.io/products

Check your developer console, no session is created yet. Then you add a product to the cart and a session is created. Then you go to the catalog page or any other page then check the response in network tab then you will see that there is no X-Drupal-Cache and X-Platform-Cache is a miss too.

Then if you delete the session cookie and reload the page then you will see that the X-Drupal-Cache and X-Platform-Cache is HIT.

This is a major performance issue, because of this if an anonymous user is casually browsing through the site after adding product to cart then every request will reach the drupal server and won't be served from adding overhead to server.

I was initially thinking it to be a commerce issue but then I saw this ticket and thought maybe its a Core issue.

I hope my steps instruction willl make it easy to reprocude the issue.

Thanks

Uploading patch with the code that filters out the current cart from carts variable. Please review.

Thanks

This still needs work.

The file path in the patch was not right. I have corrected it.

We need to discuss about the solution because this patch takes care of single cart. It takes the carts and then filter and reset the carts array to get the single cart. And then work on it.

I am proposing a way to get the cart only from the cart id.

Will be uploading the patch with updated code.

Thanks

Hi,

Patch in #7 was not applying in composer for me. Attaching the rerolled patch.

Thanks

Rerolled patch for D10.

Hi, is there any progress or update on this task?

Looking forward to use this module on a D10 site.

I think unsubscribing is not enough and we it delete/archive old entry as well.