On the other hand, the logger will be removed, once the deprecated code gets removed in 5.x. Meaning we would need to introduce a new logger object which will be introduced as deprecated... Meaning third party modules would need to take care of a second deprecation...
So all perfect, please RTBC!
Some of the warnings are incorrect, e.g. "Call to method setRouteParameter() on an unknown class Drupal\flag\ActionLink\Url." but others make sense, e.g. using a non injected logger instance.
Providing static patch for the time being.
Makes sense! LGTM!
Alright, all done!
The last remaining test failure is unrelated, as the test also fails on current 8.x-4.x-dev, as can be seen here: https://git.drupalcode.org/project/flag/-/merge_requests/68.
Please review!
Change record can be found here: https://www.drupal.org/node/3458551 → .
That is quite weird. I get the exact same phpunit error locally, when running Drupal\Tests\flag_follower\Functional\FlagFollowerUITest::testUi on the 8.x-4.x dev branch, even if the remote on "main" succeeds: https://git.drupalcode.org/project/flag/-/jobs/568309
I adjusted and moved the deprecation notices here: https://git.drupalcode.org/project/flag/-/merge_requests/67/diffs?commit...
All methods end up either calling getAsLink() or getAsFlagLink() and since these methods are all public, it only makes sense to throw the deprecation errors there.
[...] I've seen NULL as default also in core for that. Core then falls back to default if needed [...]
I couldn't find any core code doing that. It is instead probably related to @claudiu.cristea in #20:
[...]
Make it optional, defaulting to NULL, otherwise is breaking BC for all sites extending these methods
If NULL is received, we should
Trigger a deprecation error
Log a warning message
Messages should state that the param will be mandatory in the following Flag major version.
[...]
I'd agree with that suggestion and seeing that the deprecation and logging part isn't implemented yet, and one test still fails, I'll set this to "Needs work" again. I'll have a go at it.
Done!
Grevil → created an issue.
Grevil → created an issue.
Grevil → created an issue.
Fixed in current 1.0.0-beta1 release!
Currently not possible, as the library currently does not have a proper bulilt js file, when we get the library through https://asset-packagist.org/package/npm-asset/js-cloudimage-360-view.
Done.
Thanks all!
Cleaned up the existing code a bit and fixed the module file implementation. Please review!
Note, that by the official meta documentation, "value" doesn't seem to support NULL values, so let's use "0" where we can.
2.x now differs from 1.2.x. There were a couple of issues only merged for 1.2.x. We should reset 2.x on the status of 1.2.x once its time.
Merged!
Didn't change the MR title, so it doesn't show up in this issue.
LGTM! Just tested a bit around with it. Everything is still working as intended!
A similar problem arises in the module file, if the price is null:
TypeError: Drupal\commerce_price\Rounder::round(): Argument #1 ($price) must be of type Drupal\commerce_price\Price, null given, called in /var/www/html/web/modules/custom/facebook_pixel/modules/facebook_pixel_commerce/facebook_pixel_commerce.module on line 61 in Drupal\commerce_price\Rounder->round() (line 29 of modules/contrib/commerce/modules/price/src/Rounder.php).
Let's fix that here as well.
Best would be to get the "details" wrapper out of the data!
That's what the module is actually already doing in "validateSettingsForm", I am unsure based on what exactly the "'use_form' is not a supported key." text is displayed, can't be based on the saved values, as there shouldn't be a value for "use_form" saved.
But yea. Still unsure how to approach this.
@jsacksick, yes I kept that from the original patch. It shouldn't matter, as the order item is only created, but not saved nor added to the cart. And if there are any invalid fields / unallowed fields we'll throw a 422 error before adding it to the cart.
This way we can easily get the field definitions from the entity without injecting any additional services.
If you don't like it, we can go back to injecting the field manager again, like in https://git.drupalcode.org/project/commerce_api/-/merge_requests/21/diff...
Then, we can filter out invalid fields before actually creating the order item. You decide! :)
Code LGTM!
Fixed. Works as expected again.
Grevil → changed the visibility of the branch 3456986-removeemptyparagraphs-is-not to hidden.
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.