This is quite interesting, as the filter does not have any setting, nor a setting specifically called "remove_empty_paragraphs".
Grevil → made their first commit to this issue’s fork.
I tried to fix it properly in 3456700-useform-is-not, but that is quite the rabbit hole and my current approach is still not the best.
Basically, the problem is that our details wrapper "use_form" is seen as a valid setting and saved. Instead it shouldn't be saved at all, or atleast it should be seen as a sub-mapping of the settings mapping, but doing so will result in other problems.
Let's merge the quick fix for now, so this module can be used again, but keep the issue open, so once we have the time, we can continue our work in 3456700-useform-is-not.
Just tested 3456782-9.patch. It still works and test succeed as well! LGTM!
Yes, that fixes the issue!
Thank you, @jsacksick! Simply but effective fix!
@Anybody, yea this is quite the edge case, interestingly enough, if we add an item to the cart as an anonymous user, "$cart->getItems()" in line 109 of the CartManager.php, will be empty and therefore the combination of orders will succeed pretty fast. And even if we use the default commerce setup and add an identical item to the cart as an authenticated user, it succeeds, because the default order item has no price field to match.
I think this issue can be renamed. The basic gist of it is, that FieldItemList->equals(), doesn't work for price fields. No need to get into the nitty-gritty.
Grevil → created an issue.
FYI: Just found out, that the "combine" meta key doesn't properly work and "arity" is indeed not used anymore. Both also applies to current 8.x-1.x and doesn't have anything to do with this fork's changes.
I'll provide a seperate issue for that.
Alright, all done! Sorry for the noise, I haven't worked with DrupalCI for a while now and my local test environment didn't show the last phpcs error.
All desired tests were added and all tests are green. Please review!
Ah ok, thanks for clearing this up!
The given meta fields 'arity' are not valid entity fields.
I don't think "arity" is a valid meta key? There seems to be no code using "arity" and it isn't documented in https://www.drupal.org/docs/8/modules/commerce-api/cart-and-checkout/add... → .
I'll adjust the test accordingly.
Created a new release.
LGTM!
All done. Please review!
Alright, this should do the trick!
Don't think hardcording the "field_" prefix is a good idea
We are now using a similar approach to your snippet and if a base field is being set, we throw a 422 error.
Also, not a big fan of enforcing required fields to be submitted
Removed that. We are now only validating fields set through "meta".
New static patch for the time being.
Fixed Problem 2, now the user is not able to set any based fields apart from "quantity". This should be sufficient for now. But we should think about cases, where a developer would like the user to be able to override specific base field values as well.
I talked about this internally with @Anybody → and we thought about one conclusion, where we would validate the meta field values through the "add_to_cart" form mode (or a custom form mode specific to this module).
This way, an admin could easily enable / disable fields he wants to be available / unavailable through the cart add API call.
@jsacksick, what do you think about this? Should we implement it here or as a follow-up issue? Or not at all and keep the current logic as is?
Ok, I fixed Problem 1. First I thought about introducing the original payload structure, with a dedicated "fields" key for the payload.
This way we would have a clean separation between field data and all other meta properties (currently only "combine"), but this would cause a BC and nobody likes those, so I reverted it again.
Ok, I think we still have 2 problems left we need to solve:
Problem 1:
Currently, if we provide a field, that doesn't exist on the order item type, the field value simply gets skipped, and the order item will be created without the field value. I think one would expect to get a 422 error instead, as the provided field / field value doesn't exist on the entity and can therefore not be used.
Problem 2:
Now that we expose all order item fields, people can easily abuse it by overriding the unit price of the order item like so:
{
"data": [
{
"type": "commerce_product_variation--frame",
"id": "c8fb0673-9519-4e8b-a9d5-7a42e8518065",
"meta": {
"combine": true,
"quantity": 1,
"overridden_unit_price" : true,
"unit_price" : {
"number" : 400.00,
"currency_code" : "EUR"
}
}
}
]
}
Meaning we need further validation on the person doing the post request, whether he is illegible to override sensible fields.
Alright, I did some renaming and adjusted the code similar to how it was suggested by @nicer.
Please review! Works great on my side :)
Static patch attached for the time being.
@nicer concerning #20, I feel like this is a great idea!
We would once again validate fields that might not be included in $meta, BUT in this case it would greatly benefit us, as we will see if we missed a required field!
+1 from my side, I'll add it to this issue fork branch.
Whoops, the new install hook was inside the brackets of the older install hook...
Fixed it again. Static patch provided.
This module in its current state is unusable. "DevelGenerateCommands" moved from "Drupal\devel_generate\Commands\DevelGenerateCommands" to "Drupal\devel_generate\Drush\Commands\DevelGenerateCommands" but is also declared final now.
There is a different way on how to register devel_generate drush commands now, you simply have to copy the "generate" Method, where we need to call the devel_generate_commerce "CommerceDevelGenerate" plugin generate methods.
But this isn't all. Even if we delete the Command file and try to generate the commerce entities via UI ("/admin/config/development/generate/commerce"), we get more an more errors thrown, as there are quite a few deprecations in "src/Plugin/DevelGenerate/CommerceDevelGenerate.php" which need to get fixed before this module can work again.
Setting this issue to major, as the module isn't usable with devel 5.x in its current state.
Just installed the module and I get the exact same error.
LGTM!
Oh, this was already fixed through 🐛 Event data should be passed as object Fixed . I thought the gitlab diff might be buggy. Oh, well.
All green. I spoke internally with @Anybody, and he thinks the changes make sense as both D8 and D9 have reached their respective EOL a while ago.
All green and works as expected. Thanks!
My apologies @loze, your original changes were correct, while mine are not. I think I simply rushed it and came to an incorrect solution.
We always get a JSON string inside our js, which we need to parse to "real" JSON, like your original changes suggested. I'll revert my changes.
Grevil → created an issue.
Grevil → created an issue.
Btw, the commerce core "order" module has a quite similiar implementation (with some minor "performance" adjustments, which I don't think we need). See https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/order....
So this edge case will also apply there.
Here is a link of when our order processor is run: https://docs.drupalcommerce.org/v2/developer-guide/orders/getting-starte....
I think we really need to hook into the pre checkout.
Alright all done!
Are tests still needed? The functionality didn't really change THAT much, but I can understand if you still desire some.
I'd still vote for the approach in "2973129-allow-none-any-block-selection", because the current behavior, that selecting all blocks leading to $this->getConfiguration()['plugin_ids'] being empty seems not being intentional and a weird behavior of the "tableselect" form element.
Please readd the tag "Needs tests" if any are still needed.
@Anybody unfortunately documenting only won't fix the problem, when using "categories". We can only support any future category, when allowing no value checked.
Ok, funnily enough @Anybody's and mine request is already covered by the module's functionality. Through selecting all blocks, any newly added blocks will be automatically available (and checked on the settings page).
This is quite unusual for Drupal, as generally the logic for these kinds of form elements is, that if everything is unchecked, all given blocks will be available.
Meaning we would have two approaches to fix this issue:
- Make the block selection not required, allowing the user to have all blocks available through either selecting all or none blocks on the settings page.
- Document the selection behavior on the settings page.
Just tested the current implementation and it doesn't work anymore.
I am still fairly sure, it has its use case. But we should make sure, that the user is not able to select block, that would lead to errors / recursion. Of course, tests should be added as well.
This should do the trick. Feel free to test it @loze!
Yea, this is quite weird, the approach here is incorrect though:
Inside "facebook_pixel.module" line 51, we have the following code:
'data' => Xss::filter(json_encode($event['data'])),
This doesn't make much sense as Xss::filter returns a string. Instead, we should turn the statements around.
Drupal 8.9 as well as Drupal 9 reached its EOL, we simply forgot to drop support.
Makes sense to me and patch LGTM!
Have you already tested this? Also I am quite confused, that both events are not documented in "commerce/modules/order/src/Event/OrderEvents.php".
This fixes the error thrown. But I am not 100% sure, if "NULL" is a good fallback here.
@maxilein yes, this is testable if you are using 10.3.x! Just apply https://www.drupal.org/files/issues/2024-06-04/2230909-325.patch → through composer! :)
@smustgrave probably... I don't have the permission to close https://git.drupalcode.org/project/drupal/-/merge_requests/579, unfortunately. Anybody with proper permissions, feel free to close it in favor of https://git.drupalcode.org/project/drupal/-/merge_requests/8282.
Accidentally pushed to 2.x. Ready to review though!
Alright, the field storage should be updatable now, through the in 10.3.x newly introduced "updateFieldStorageDefinition()" method!( https://www.drupal.org/node/3397892 → )
Please review (no idea why the review bot set this to needs work, it definitely applies on 11.x).
Here is the new static patch (Update hook won't work for Drupal < 10.3.x).
Tests are all green now! Ready to get reviewed. 👍
Looks like everything works as expected! Let's wait for the tests to succeed.
Here is a static patch of the current MR for the time being (which also applies to 10.2.x, 10.3.x and 10.4.x).
[...] and add the test from 305 and review against that
I think you meant the update hook provided in #305? Unfortunately, the update hook from #305 is pretty much a template implementation and won't work in its current state. I'll give it a shot.
The hook_requirements in 296 hasn't been addressed either I don't think
I'd agree with @pwolanin in #298:
@anypost - this important patch has been waiting for 7+ years to get committed. we should not expand the scope [...]
Any further work on this should be done in a follow-up issue.
https://git.drupalcode.org/project/drupal/-/merge_requests/579 still has their target on 9.2.x and can be removed safely.
New MR targeting 11.x can be found here: https://git.drupalcode.org/project/drupal/-/merge_requests/8282.
Easy and simple fix. Same implementation as for media in core (see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/media...).
Please review!
This is actually a "commerce_product" issue and unrelated to "commerce_xattributes". I'll move this issue.
Added a few comments!
Changes LGTM! PHPStan nor PHPCS are throwing any errors anymore!
Fixed by @LRWebks in the parent issue.
Fixed by @LRWebks in the parent issue.
Fixed by @LRWebks in the parent issue.
Reversed first merge, as the MR still targeted the wrong branch.
Alright, great stuff! @LRWebks just confirmed internally, that the update hook also works as expected! :)
Let's create a first alpha release! Great work, thank you! 🎉
If one of the maintainer feels like the tests are too far out of scope, we can always split the test adjustments, but they are definitely needed!!
Ok, I just took a quick look at the tests and I am shocked at the state of them. "testJsInjector()" doesn't even test any javascript, but css instead, and only whether the css files exist, not even if the css is applied correctly...
So I'd say you do a quick test overhaul here. Create a new Functional Javascript test class "AssetInjectorFunctionalJsTest", which contains two tests "testInjectedJs" and "testInjectedCss".
For "testInjectedCss", inject some css and test if it is injected correctly (Mink doesn't have specific css tests, but the evaluateScript() method will help, similiar to how it was done in https://stackoverflow.com/questions/25494456/is-it-possible-tests-css-st...).
You can keep the old "testCssInjector" test.
For "testInjectedJs", Inject a js alert for the front page only (""), go to the front page and await the alert (there is a driver method for this). And please delete the old "testJsInjector" method.
Both tests should also revisit the page and see if the css / js is NOT injected anymore, after the asset entity's status is set to false. This way, this overhaul isn't too much out of scope 😛.
Tip: Create the the entities programmatically for better performance! 😊
Code LGTM!
Could you finally reinstall the module and do some manual testing of all the new features (additionally some human error testing, e.g. blanks set incorrectly, defined mails incorrectly, sepereated with "." instead of "," etc.?
Added a few comments, back to NW!
Thank you, @jcnventura!
Sorry, I thought you might not be eligible to add any (co-)maintainers. Appreciate the fast reply! I'll start cleaning up the issue queue soon! Feel free, to also add @Anybody, I usually work very close with him in terms of code reviews etc. as he's quite knowledgeable (and my co-worker).
I contacted @yannickoo 13 days ago without any feedback, I guess it is eligible to be moved now, even if we did not wait the full 14 days?
Link to the module: https://www.drupal.org/project/menu_link_attributes →
While we are at it, we should also fix the failing tests!
Important quick fix! LGTM!
6 tests fail. Seems that we don't have a .gitlab-ci.yml in this project, so the failing tests are not appearing on the main page. Let's fix the tests and other possible deprecations here.
@hideaway
First of all it broke our builds as latest version does not apply to latest D10
I am sorry for that, but stuff like this can always happen. I generally wouldn't recommend to use branch diffs as patches for composer (because of their dynamic nature). Instead, always make sure, that a static patch is provided!
Alright, branch is back on track with 11.x + issue related changes.