- Issue created by @quimic
- Status changed to Postponed: needs info
9 months ago 4:31pm 9 February 2024 - πΊπΈUnited States nicxvan
Can you try providing the token name of the entity that you are working with?
If that is it maybe we can work on the error messaging.
- π«π·France quimic Paris
@nicxvan Whatever value I enter in the entity field, I get the same error message when saving. No entry in the log.
- π«π·France quimic Paris
I decided to see if I would get a similar error using the Classic Modeler, and indeed I do, but I get a little more details on what might cause it.
When I use the "Order Paid" event and try to add the "Commerce: Order contains product variation types (ECA Commerce)" condiftion, I get a white page with the "The website encountered an unexpected error. Please try again later." message.
Looking in the log, I see "TypeError: Cannot access offset of type string on string in Drupal\eca_commerce\Plugin\ECA\Condition\Commerce->filterFormFields() (line 145 of [...]/modules/contrib/eca_commerce/src/Plugin/ECA/Condition/Commerce.php)."Using the "Commerce: Order contains product types (ECA Commerce)" brings the exact same error.
Hope this helps.
- π©πͺGermany jurgenhaas Gottmadingen
As soon as I find some time, I'm going to debug the form validation error.
Using the classic modeller is a bad idea, since it doesn't validate the configuration at all. The other error you're seeing is something that is something different, but it could also be a follow-up on this one.
- Status changed to Active
9 months ago 2:06pm 13 February 2024 - π©πͺGermany jurgenhaas Gottmadingen
Found some time to debug the issue, and it turns out that the "Type" field is not a plain textfield as it appears in the UI. The original condition plugin from commerce uses a field type called
commerce_entity_select
. That's of course not supported by the bpmn_io UI and we therefore replaced that with a textfield, see β¨ Add support for commerce conditions Fixed .That replacement now seems to cause the issue with the form validation, which still expects the complex field widget instead of the simple textfield.
I'll have a look if I can make that replacement more solid.
- Merge request !8Issue #3420468 by jurgenhaas, quimic, nicxvan: "Field xxx is required" error... β (Merged) created by jurgenhaas
- Status changed to Needs review
9 months ago 2:48pm 13 February 2024 - π©πͺGermany jurgenhaas Gottmadingen
It seems that the commerce conditions did never receive any configuration values from the bpmn UI so far, not only those
commerce_entity_select
were broken. Turns out, there have been 2 issues:The first issue was the handling of the
commerce_entity_select
fields. The commerce conditions left it to the bpmn_io modeller to replace that unsupported field type with the fallback of a text field. That works for the UI, but not for their validation. To resolve this, I've now replaced that complex field type with a text field in the already existing\Drupal\eca_commerce\Plugin\ECA\Condition\Commerce::filterFormFields
method. That resolves the validation issue.However, the second problem is that commerce condition forms are somehow nested in a way, that I never come across before. That leads to a problem where their config submit handler reads values from the form state with
$values = $form_state->getValue($form['#parents']);
, but that returnsNULL
in our context. The submitted values are available, but not at the expected place, so I've fixed that by setting the values with$form_state->setValue($form['#parents'], $form_state->getValues());
before calling the submit (and also the validate) handler.It now seems to work. Not sure if this is just a terrible workaround or a valid fix!?!
- π«π·France quimic Paris
Thank you Jurgen. I would not be able to assess if this is the right fix.
I can confirm that after applying the patch, the form validates and I can now save the model I described (display a message if a paid order contains a specific product variation type).
But when I trigger the model (by paying an order), I get an error from the commerce module:
TypeError: in_array(): Argument #2 ($haystack) must be of type array, string given in in_array() (line 36 of [...]/modules/contrib/commerce/modules/product/src/Plugin/Commerce/Condition/OrderVariationType.php) - π©πͺGermany jurgenhaas Gottmadingen
Thanks @quimic for the quick feedback. I can see the reason for that:
commerce_entity_select
expects an array of values, not only a single text value. So, we may have to extend that field in a way that it allows multiple values (i.e. a single string with comma separated values) and then explode the given value into an array of strings. I'll see now, if I can make that happen. - π©πͺGermany jurgenhaas Gottmadingen
I've applied a method that has also been used for the eca_tamper module: we verify the data type of each configuration value and adjust that if necessary before calling the commerce condition plugin to evaluate the conditions. This should do the job. Please note, that the config field now also has a description that should explain that you can provide multiple values separated by commas.
- π«π·France quimic Paris
Thank you Jurgen.
I now get an "opposite" error, from ECA Commerce. It complains about receiving an array instead of a string.
TypeError: explode(): Argument #2 ($string) must be of type string, array given in explode() (line 71 of [...]/modules/contrib/eca_commerce/src/Plugin/ECA/Condition/Commerce.php) - π©πͺGermany jurgenhaas Gottmadingen
Hmm, I need to fly pretty blind here since I'm not a commerce user and I don't have any commerce installation where I could debug this. I've now added another check before converting a config value from string to array. Hope that catches it and doesn't introduce any unwanted side effects.
- π«π·France quimic Paris
Okay, so now there is no error any more.
BUT, the condition of the sequence flow should have matched and it did not.
More specifically:
I payed an order that contains a specific product variation type
I check for that variation type in my model, and display (or log) a message if it matchesI did not get the message. "Negating" the condition in the sequence flow does give me the message.
So it means that the values comparison fails. - π©πͺGermany jurgenhaas Gottmadingen
Then you may have to go through debugging as described in https://ecaguide.org/eca/debugging/ which works completely in the UI, i.e. in your browser.
Even better would be if you could use XDebug, to step through the code to see where it goes wrong. If that is an option, the ideal starting point would be
\Drupal\eca_commerce\Plugin\ECA\Condition\Commerce::evaluate
which is what's called when a condition gets evaluated. This then calls$commercePlugin->evaluate()
from the respective commerce plugin where the real evaluation is happening.Hope that either of those 2 options provides us with more detail what's going on.
- π«π·France quimic Paris
It looks like the Commerce tokens give me inconsistent values, so I need to investigate further to understand what is happening here.
- Status changed to Postponed: needs info
9 months ago 4:41am 29 February 2024 - πΊπΈUnited States nicxvan
Please let us know if you find where you're getting the inconsistencies.
Hi, jurgenhaas, the solution has not been released to production or any release, right?
Regards,
- π©πͺGermany jurgenhaas Gottmadingen
@sdsc the fix exists in the MR/patch and will be merged, when it's been tested and confirmed that this really fixes the big.
I'll wait for your next version release -- sorry, I don't know how to apply MR/patch π , I am not technical and don't want to break code in case I did anything wrong.
When would you expect next version update(with this fix merged)?
Thank you again for making ECA available while Rules is really useless at this point...
- Status changed to Needs review
7 months ago 7:51am 6 April 2024 - π©πͺGermany jurgenhaas Gottmadingen
@sdsc are you running a Drupal site without a single patch? I've never seen such a thing, TBH. Our Drupal sites have a few dozen if not a hundred patches applied.
Assuming you're building your Drupal site with composer, it should be very easy. Detailed instructions can be found at https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... β
The link to the patch you want to apply is below the original post above, left to
MR !8 mergeable
behind the text "plain diff".When would you expect next version update(with this fix merged)?
As soon, but not before, someone who hasn't developed the fix, will have tested it and can confirm, that it really works and doesn't break anything else.
jurgenhaas, oh, yeah, embarrassing, right? My D7 site is actually very complex, with shopping, membership, lots of content type (LOTs of content) and internal request/orders...yet no patches (sorry π)
Now I am in the process of building D10. Really headaches about: Rules, Ubercart to Commerce, and many good modules and features gone...Luckily ECA is coming to replace Rules, with many advantages, yet some room to fill up. The great simplicity and flexibility in Ubercart are much not available in Commerce, ECA is probably my last resort to get solution.
I'll definitely try the patch method per your advice. But commerce and its related modules already broke part of code. Now I am starting a new site just to test all required commerce features.
Thank yo again!
- πΊπΈUnited States nicxvan
If you're building a commerce site you'll want to get familiar with patching.
For commerce 2 many companion modules will require patches to fix minor things.
As Jurgen said once someone confirms the patch fixes the issue we can merge it in, it won't likely get merged in otherwise because we cannot be sure it fixes the issue and or introduces other issues.
I am unable to apply the patch. I must have done something wrong:
1. go to https://git.drupalcode.org/project/eca_commerce/-/merge_requests/8.diff and copied content to a txt file, name it as eca.patch
2. created a directory "patches" at the same level as core
3. moved the eca.patch to the folder
4. add the bolded section to composer.json:
"extra": {
"patches":{
"drupal/core":{
"fix issues of check commence order as condition": "patches/eca.patch"
}
},
"drupal-scaffold": {
"locations": {
"web-root": "./"
}
5. run "composer install --prefer-source". Error:- Applying patches for drupal/core
patches/eca.patch (fix issues of check commence order as condition)
Could not apply patch! Skipping. The error was: Cannot apply patch patches/eca.patchDouble checked the folder name and file name. Please advice.
- πΊπΈUnited States nicxvan
You're really close i think you're missing two things:
1. You're patching eca_commerce not core so the party that says drupal/core should be drupal/eca_commerce
2 make sure you have required cweagans/composer-patches Yes, I did.
1. Before I updated the patch, I did "composer require cweagans/composer-patches:~1.0 --update-with-dependencies". It asked me whether to trust"...", I typed "y". Looks like this action was performed successfully.
2. then "composer update drupal/eca_commerce", with the same error,
3. then I tried "composer install --prefer-source"- πΊπΈUnited States nicxvan
Yes, the part in composer json where you have drupal/core needs to be drupal/eca_commerce
That cost key needs to be the project you are patching.
Hi, nicxvan,
I changed to "drupal/eca_commerce" in composer.json file, but still with the same error:
- Applying patches for drupal/eca_commerce
patches/eca.patch (fix issues of check commence order as condition)
Could not apply patch! Skipping. The error was: Cannot apply patch patches/eca.patchIs it possible the patch file I generated (copy from the site to txt file) is not good? I loaded it here,, just in case.
Thank you for your patience!
- πΊπΈUnited States nicxvan
Yes, that patch is not correct, it should be equal to the content on this page: https://git.drupalcode.org/project/eca_commerce/-/merge_requests/8.diff
If you notice in your patch there are a ton of extra / and a whole section at the top:
{\rtf1\ansi\ansicpg1252\cocoartf2758 \cocoatextscaling0\cocoaplatform0{\fonttbl\f0\fmodern\fcharset0 Courier;} {\colortbl;\red255\green255\blue255;} {\*\expandedcolortbl;;} \margl1440\margr1440\vieww11520\viewh8400\viewkind0 \deftab720 \pard\pardeftab720\partightenfactor0 \f0\fs26 \cf0 \expnd0\expndtw0\kerning0
I noticed two other things.
One, you probably need ./patches as the relative path and two, you may need the dev version of eca_commerce since patches are generated against the dev version and not the current version.
If this does not work please open a separate ticket asking for support to apply this patch. I'm willing to help there, but I don't want to clutter this issue up with support comments.
Thank you Nicvan! I was able to apply the patch and did some testing. Here is where I got:
1. Order contains product types
-->saved successfully
2. Order contains product variation types
-->saved successfully
3. Order contains specific products
-->put product id in field "Products": failed to save
-->put product title in field "Products": saved successfully
-->put a wrong product title in field "Products": failed to saveMay I suggest:
a. use product id for field "Products" so editing product title will not break the code? It would be nice to add a description there.
b. If entered data in products field doesn't exist, the code should still be saved and working, because it is possible that a product was modified or removed later. I think the code should be able to handle "not found" situation.4. Order contains specific commerce purchasable entities
-->seems not developed yet.
5. Order contains product categories
-->not sure what it is, but errors pointing to term matching not foundLet me know if you need more testing regarding this patch.
- π©πͺGermany jurgenhaas Gottmadingen
This is great news, thanks @sdsc for testing all this.
Note, the configuration forms are implemented in commerce. ECA Commerce is just pulling the plugin that exist in commerce and makes them available in ECA. So, the labels, descriptions and validations are out of scope for eca_commerce. Any changes would have to be made in the commerce module, that provides those plugins.
3. Order contains specific products
-->put product id in field "Products": failed to save
-->put product title in field "Products": saved successfully
-->put a wrong product title in field "Products": failed to saveCan you provide the exact error messages for the 2 failing attempts, please?
the error for the two scenario is the same:
(Flow_olkv32k): There are no products matching "3" -- 3 is what I put in the field. The code is looking for a product with title "3'.
ECA Commerce is just pulling the plugin that exist in commerce and makes them available in ECA. So, the labels, descriptions and validations are out of scope for eca_commerce.
--is it possible commerce has another filed for product ID that ECA Commerce can pull to check against? I don't know the coding, just from experience perspective, it is usually compare to machine name that is relatively fixed rather than an editable field.
Thank you for taking effort to make your module better and better!
- Assigned to jurgenhaas
- Status changed to Needs work
7 months ago 4:35pm 8 April 2024 - π©πͺGermany jurgenhaas Gottmadingen
Thank you @sdsc for that error message, that helped me to find out this:
MR !8 fixes the issue with config field of type
commerce_entity_select
and with commerce plugins in general.The conditions "Order contains specific products" and "Specific products" use another config field type
entity_autocomplete
, which isn't available in BPMN. We need to see, if we find a way to support that config field type as well.Would be great if you could test this further, when I'm ready with new code, since I'm not a commerce user, i.e. I don't have real sites where I could run those conditions. My coding is just based on theory, so if you can test something every now and then, that would be amazing. I can't make any promises by when I could look into this in more detail, though.
Glad to help!
entity_autocomplete should refer to the title. It's surprising that they don't have a field for product_id for eca to check against. But if it is what you can get, would be nice to put a description there, because the normal thought would be product_id.
- Issue was unassigned.
- Status changed to Needs review
7 months ago 10:13am 17 April 2024 - π©πͺGermany jurgenhaas Gottmadingen
The fix is probably as simple as treating the
entity_autocomplete
similar to thecommerce_entity_select
, i.e. that would allow comma separated list of IDs.I've added that to the existing MR, please give that another try.
@jurgenhaas, didn't know you were waiting for my response.
one or more commerce related module broke my code, and couldn't fix. So I had to rebuild my site and import all data.
I am now very cautious when it comes to commerce. ECA has helped me A LOT to make up some features missed from Commerce. I haven't got a chance to re-apply the patch yet. I'll let you know as soon as possible.
Thank you again for making ECA available!
-
jurgenhaas β
committed 14114af3 on 2.0.x
Issue #3420468 by jurgenhaas, sdsc, quimic, nicxvan: "Field xxx is...
-
jurgenhaas β
committed 14114af3 on 2.0.x
-
jurgenhaas β
committed 71a475ea on 2.0.x
Issue #3420468 by jurgenhaas, sdsc, quimic, nicxvan: "Field xxx is...
-
jurgenhaas β
committed 71a475ea on 2.0.x
-
jurgenhaas β
committed 5b3b2e09 on 2.0.x
Issue #3420468 by jurgenhaas, quimic, nicxvan: "Field xxx is required"...
-
jurgenhaas β
committed 5b3b2e09 on 2.0.x
-
jurgenhaas β
committed d83d12d6 on 2.0.x
Issue #3420468 by jurgenhaas, quimic, nicxvan: "Field xxx is required"...
-
jurgenhaas β
committed d83d12d6 on 2.0.x
-
jurgenhaas β
committed 4038566a on 2.0.x
Issue #3420468 by jurgenhaas, quimic, nicxvan: "Field xxx is required"...
-
jurgenhaas β
committed 4038566a on 2.0.x
- π©πͺGermany jurgenhaas Gottmadingen
This is now also merged into the 2.0.x release. For 1.0.x this is still living in the MR waiting for the review.
Today I had a chance to test 1.0.x dev in my testing environment. I successfully applied the patch, but for "containing specific product" condition, It appears the same as before, not presented as "entity_autocomplete".
I am unable to update to 2.0.x alpha release somehow. My testing environment may have something broken from other commerce related modules. The condition of "containing specific product" works as long as the product is a full name of product title. This should be fine.
Thank you very much for working hard to make ECA better and better!
-
jurgenhaas β
committed 627e81bd on 1.0.x
Issue #3420468 by jurgenhaas, sdsc, quimic, nicxvan: "Field xxx is...
-
jurgenhaas β
committed 627e81bd on 1.0.x
- Status changed to Fixed
6 months ago 11:22am 24 May 2024 - π©πͺGermany jurgenhaas Gottmadingen
I've now merged this to make it part of the 2.0 beta1 release. Also marking the issue as fixed. If any more findings need to be reported, please open new issues for those.
Automatically closed - issue fixed for 2 weeks with no activity.