Forgot to update the unit tests. Should be all green now! Fixed!
Here is the MR: No idea whats going on with drupal gitlab:
https://git.drupalcode.org/project/user_preference_login_redirect/-/merg...
Added a new Drush Command, put the parser inside a service and added a README. Please review!
I don't get the Test failures, and I don't want to spend half a day fixing them.
The tests all are green locally. And testing the flow manually, everything works as expected. Merging with commented out test.
SettingsForm.php and in FormHooks.php
I guess you meant "FormHooks" and "UserHooks"? Yea, we only have one hooks file and we only call it once now. The second one was completely unnecessary IMO (checking, if the login preference still exists in the current config).
Done. Please review!
Works as expected! On Login I get redirected to "https://standard-vscode-drupal.ddev.site/test/bla?another_param=456&chec...", when adding and using "entity.node.canonical/node:1/another_param:456|Test" in configuration.
I simply wonder how it ever was called. No parent extends "ContainerInjectionInterface" and the service definition:
services:
views_menu_children_filter.join_handler:
class: Drupal\views_menu_children_filter\Plugin\views\join\MenuChildrenNodeJoin
arguments:
-
type: INNER
table: menu_link_content_data
field: false
left_table: false
left_field: false
operator: '='
- menu_children_node_join
- ''
generally uses "__construct()" directly. Nor have I seen this pattern in any other "ViewsJoin" Plugin.
Please test the new MR changes, if everything still works as expected. I don't have the time right now, to test the changes.
I'm unsure if it needs the option to select different payment method logos in the block instead of simply selecting the payment gateway to use
I'd say, it is good to have both options, e.g. for cases where there are multiple payment gateways configured (of course, the payment methods need to be supported by the block in that case, to make this feature make sense).
I adjusted the name accordingly. Here are some screenshots of the settings and output:
Settings 1:
Settings 2:
Output:
Please review once again!
That's it, Please review!
Note, that the tests succeed, even if the pipeline fails. The pipeline failure is related to Drupal 13 incompatibilites (e.g. "Using @Constraint annotation for plugin with ID State is deprecated and is removed from drupal:13.0.0").
Hey Jonathan!
Just tested it locally with the new changes. Still works as expected!
Thanks for taking the time! 👍
All done!
Tested requiring via asset packagist and composer merge plugin. The status reports look as expected, when the library is installed / uninstalled. Also tested installing the library, but removing the min.js.
Library installed:
Library is missing / min.js not found:
I don't quite get this plugin implementation conceptually.
For one, there are two instantiation mechanisms for the same plugin:
One, as a service ("views_menu_children_filter.join_handler", which is used by several other classes in this module) and the other through the views plugin system (via `@ViewsJoin("menu_children_node_join")`)
Second, Neither will actually use this "create" method:
The views plugin system instantiation would only use "create", if the class would implement "ContainerFactoryPluginInterface" but the class doesn't nor any parent nor any other class extending "JoinPluginBase" in core ever used dependency injection on a join plugin.
The service handler instantiation uses "_construct" by default.
Meaning, this method was never actually called.
Also, the create implementation doesn't actually inject any services. It is only used to set default settings which were never used to begin with.
I'd vote for entirely removing the method (and instantiating it only as a service? Unsure about that, not too deep into views implementations). It is not even manually executed where the class is instantiated as a service. Though, I wonder how the error:
Fatal error: Declaration of Drupal\views_menu_children_filter\Plugin\views\join\MenuChildrenNodeJoin::create(array $configuration, $plugin_id, $plugin_definition) must be compatible with Drupal\Core\Plugin\PluginBase::create(Symfony\Component\DependencyInjection\ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition)
came to be. Not even in 11.3.x "PluginBase" has a create method, see https://git.drupalcode.org/project/drupal/-/blob/11.3.x/core/lib/Drupal/...
The code inside the MenuChildrenNodeJoin Plugin looks very "unusual", let me take a look.
This is already implemented!
It doesn't check, if the "js-cookie" library has a "dist" folder set, but it DOES look for the "js-cookie" library folder. This should be enough. No idea how we required a library without the dist folder 🐛 php error Active . That shouldn't happen.
No, you can follow both links if you want:
https://asset-packagist.org/package/bower-asset/js-cookie
https://registry.npmjs.org/js-cookie/-/js-cookie-3.0.5.tgz
Both contain the dist folder.
Can't reproduce this. There was no new js-cookie release. Latest version is still 3.0.5, same as the in the README recommended version.
The dist folder still exists in both the bower asset package, as well as https://registry.npmjs.org/js-cookie/-/js-cookie-3.0.5.tgz when requiring via composer libraries plugin.
I created the "follow-up" issue here: 🐛 Libraries with attributes that are solely used as dependencies for other libraries throw an aggregation error Active .
@catch note, that we have two libraries having an ID attribute set through their library definition, but only the one, which is solely used as a dependency (and not loaded directly) is having this issue and causing the "Exception: Error trying to optimize JavaScript asset" error.
Fixed!
@anybody, just a quick site note. Please always name your MR properly on creation. I usually rename them before merging, when I see, that they are not properly named, but sometimes I forget (e.g. in this case).
I restored the old tests and adjusted the selectors. All green now!
Created 3.0.0: https://www.drupal.org/project/watchdog_mailer/releases/3.0.0 →
Thanks all!
Not a big fan of the tests. But I need to take a deeper look if I got the time. Nothing seems to directly check the menu link attributes. Only permission and settings form tests...
Added a few comments for now.
Is this ready for review? Status is still set to "Needs work", but tests were added. Also for some reason the test pipeline did not start testing?
I know this is quite unrelated, but also remove Drupal 8 and 9 support. We should allow 10.5 as minimum.
Tests are not failing upstream. Only phpcs. And here phpunit succeeds, but we have failures for phpunit_next_minor and phpunit_next_major.
Possible, that the tests haven't run on dev for a while now, that's why we don't see them on the main repo.
Created a new release: https://www.drupal.org/project/ad/releases/11.0.0-alpha6 → .
Alright, this works great together with 📌 Attempt to break part tour.js Active !
Just one minor adjustment, we need to use "tour.tour_settings" as the base route, otherwise the translation tab won't be created. I manually adjusted the weight, so it looks how it looked before:
EDIT: Sorry wrong screenshot.
That's it. RTBC!
This works perfectly and fixes our issue from 🐛 Tour strings are not translatable, should be "label" in schema Needs work ! Thanks @smustgrave!
Code changes LGTM as well! RTBC!
I really don't understand the issue, especially since it is always referencing cdn.js...
We are not conditionally adding the cdn.js anywhere! It is always set as a dependency of init.js. If it were for init.js then "posthog_js_page_attachments" with its conditionChecker check, could be the problem. But it's only cdn.js!
I'll provide a patch entirely removing "hook_library_info_alter()", which we can test online first, since we are using CDN anyway. This is quite the shot in the dark, but I have no other idea.
Yea, all in all this LGTM! RTBC!
Are you able to merge this @vmarchuk? Once this is merged, I can add the PayPal support in 📌 Add PayPal support to Payment Element Gateway Active .
@vmarchuk I can't reproduce this behavior.
Went through the standard checkout, then back to the cart and bought the product through the express checkout while debugging. The ExpressCheckoutController, "onPaymentConfirm" clears the checkout data as expected before continuing, and you can even see that reflected in the stripe logs:
I created a new [...]-2.x branch with a new MR, based on the original issue branch.
grevil → changed the visibility of the branch 3421392-add-paypal-support to hidden.
grevil → changed the visibility of the branch 3477028-stripe-express-checkout-with-3421392-add-paypal-support to hidden.
Hey @vmarchuk!
For us, it is working great! I created a branch combining this issue together with 📌 Add PayPal support to Payment Element Gateway Active , which I just did some successful testing on! I'll check on the MR comments again and try to highlight the important once.
we need to clear the checkout data set during regular checkout when the pop-up opens
I have only tested express and regular checkout in isolation with this patch. What exactly are the steps to reproduce? Do you mean going through the express checkout, then cancelling it and going into the regular checkout?
Here is a 3477028 x 3421392 "franken-patch" for the time being.
Also, there were no specific tests for this issue added... but I really don't wanna waste more time here. Please review.
Added tests. Although, one will fail, because of 📌 JS advertisement type does not yet actually run the JS Active .
Not really worth it using AI to generate the tests. Took too many manual adjustments and therefore too much time...
Might need a better prompt.
One test will fail, because ad_content_js doesn't execute js. See 📌 JS advertisement type does not yet actually run the JS Active .
Pipeline display is stuck. Pipeline is green: https://git.drupalcode.org/project/jsonapi_role_access/-/pipelines/666020. Merging....
All right, all done. Quite a bit of refactoring, but everything is pretty straigth forward, except the removal of the javascript, but hear me out on this one.
Conditionally hiding / showing checkboxes has many problems:
- Roles that are checked, but hidden through js still exist in config.
- Error prone, (see 🐛 Role hiding assumption is too optimistik Active ).
- Too opinionated: anonymous, authenticated and admin roles can be whatever the website creator wants it to be. This module only covers the default configuration for these roles.
Also, the user is now also able to select the "administrator" role. Unsure, why this was filtered out from the options.
Please review!
Ok, that should be it. I'll do some code refactoring in a follow-up issue.
Nice, the test doesn't fail at line 76 anymore, now it fails at line 86. I will continue there tommorow.
I think the main issues were, that the authorization header was missing and the route builder did not rebuild after we created our content type.
But I'll leave the rest of the changes in as well, so the tests are more in line with the core json_api tests.
LGTM, and works as expected, just added a few comments regarding the tests.
Remaining test failures should be fixed in a follow-up issue. Otherwise there will be too many unrelated changes here.
Please review!
For @anybody and I the problem was the "Watchdog Mailer" module.
We injected the "@date.formatter" (DateFormatter.php) service into the logger. But loggers seem to run way too early for "DateFormatter", resulting in this issue.
Here is the related issue: 📌 "WatchdogMailer" logger runs too early for @date.formatter to be injected, causing a fatal error on cache invalidation. Active .