🇩🇪Germany @Grevil

Great work! Code wise this looks great :)

Two things, I think this would be really nice as a small (but helpful) addition:

1. Show the widget setting as ghost text on the related formatter setting input field (if empty).
2. Show whether a setting is overriden in the summary.

But I am unsure, whether this is easily implemented.

Test is green again.

Ok, this should be it, let's wait for the test output. Thanks for your work @nidhi27!

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

Works great! Good stuff, thanks. :)

grevil → changed the visibility of the branch 3438838-validate-paths-like to active.

grevil → changed the visibility of the branch 3438838-validate-paths-like to hidden.

This should be it. Please review!

grevil → created an issue.

Took a first stab on this. @thomas.frobieter should finalize it!

grevil → created an issue.

Created an issue for an easier approach here: 📌 Allow the table widget to display field groups as a dedicated column Active .

grevil → created an issue.

Note, that I can not give credit to @idiaz.roncero, because I am no maintainer.

Ah, I see. That is not really the requirement @anybody and I have in mind. I think the nuxt example @anybody gave might be a bit misleading.
We basically want to have a quick way, to edit paragraph values inside the widget, without scrolling endlessly (for paragraphs with a lot of fields). So the thought was, to have the table displayed like in the formatter output and be able to edit each paragraph through unfolding each row, showing the desired paragraph widget form(or in the case of the nuxt example a json, which would be less prefereable).

For now, it would be enough, to simply support field groups. I will create a seperate issue for that, but we should keep this issue open.

Note, that we do NOT provide the controls setting. This is a setting provided by the parent formatter "FileMediaFormatterBase" (at least for the "VidstackPlayerVideoFormatter", "VidstackPlayerRemoteVideoFormatter" extends FormatterBase, so we simply copied and pasted the setting from "FileMediaFormatterBase"). Meaning we would need to unset that setting in VidstackPlayerVideoFormatter, remove it in VidstackPlayerRemoteVideoFormatter and provide a setting with a similar name (e.g. "overlay_controls") in the Formatter Trait.

We could also modify the existing setting, but it might mess with the parent methods.

Ok, that should be it! Please rereview!

We are not using simple mega menu anymore in new site installations! So maybe somebody else can fix it.

Done, please review!

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

@lazzyvn could you go in depth on how to actually accomplish this?
I tried downloading and enabling https://www.drupal.org/project/bootstrap_table → , but I still seem to not be able to change the table looks:

Thanks for the merge @lazzyvn! Will there be a new release containing this change soon? Also, should we set this issue "Fixed"? It is still set to "Needs Review"?

Sorry for the noise.

grevil → changed the visibility of the branch 3519747-provide-an-inline to hidden.

Ok, it seems we have to use custom css anyway...

For some reason the class is not set on the container yet. I am not a style guy, so I don't do that very often. Also the css file is just some rude mentally generated AI stuff and far from being finished.
Note, that this should be the correct way to set the class on the container. The parent class does this similiarly:

    $element['attributes'] = [
      '#type' => 'value',
      '#tree' => TRUE,
      '#value' => !empty($items[$delta]->options['attributes']) ? $items[$delta]->options['attributes'] : [],
      '#attributes' => ['class' => ['link-field-widget-attributes']], // <= This is the code in question.
    ];

@thomas.frobieter would you be able to finish this?

I don't think we should use a setting here nor use custom css, but rather work with drupal native classes.

I'd agree with @mark_fullmer's comment in #5 ✨ Provide an inline display field widget setting Active . We have two options:

1. The fields will be inline by default and responsively become two lines if there's not enough space
2. We put the URL and Title inside a fieldset

I'd vote for option A, what do you think @anybody, @thomas.frobieter, @mark_fullmer?

Although we should wait for 🐛 Micon Link / Linkit / Linkit Attributes Formatter ignores icon position setting Active to get resolved.

grevil → created an issue.

grevil → created an issue.

Alright, please review! This is definitely needed. Further infos in the related issue.

Problem was the composer.json.

This will probably be better now, let's wait for the ci.

All done, please review!

Thank you, @jan kellermann! I'll test it later this day! :)

At first I wanted to vote against splitting the functionality in 2 seperate modules. But we already have a split for link supporting submodules (micon_link and micon_linkit), so another micon_linkit_attributes submodule wouldn't be too much I guess.

grevil → created an issue.

Ok, please review then!

Also, I am a bit unsure, when the Remote Formatter should be applicable:

  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    if ($field_definition->getTargetEntityTypeId() !== 'media') {
      return FALSE;
    }

    if (parent::isApplicable($field_definition)) {
      $media_type = $field_definition->getTargetBundle();

      if ($media_type) {
        $media_type = MediaType::load($media_type);
        return $media_type && $media_type->getSource() instanceof OEmbedInterface;
      }
    }
    return FALSE;
  }

Especially the "OEmbedInterface" part makes this very restricted to use.

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

Re!

Done, please review!

We can not use the Trait unfortunately, as it doesn't implement "settingsForm" as stated there. But I agree, this should be fixed.

The problem is, that the settings are not the same for both Formatters. Meaning we would need to rename the "settingsForm" method on import for one of them anyway.
We should leave it as is.

Maybe we could detect the "merge" some way

Yes, exactly this needs to be done.

Ok, this is actually a bit tricky to fix.

We basically have no info in "commerce_add_to_cart_confirmation_page_attachments", that the currently added order items are being added through the "commerce_combine_carts/src/CartUnifier.php" "combineCarts()" method:

This is because we simply add it just like how a user would add an item in their cart:

$this->cartManager->addOrderItem($main_cart, $item, $combine, FALSE);

That method triggers "CART_ENTITY_ADD", which records the added order_item inside this modules tempStore, which we call inside commerce_add_to_cart_confirmation_page_attachments again.

Inside that event we have access to:

$cart, $purchased_entity, $quantity, $saved_order_item

So we somehow need to add a flag on one of them (preferably the cart):
$cart->set('combined', TRUE)
So we can know in this module, that this cart was combined before.

Maybe it makes sense to actually flag the individual order item being added. As that is actually checked:

  $cart_item_info = $cart_confirmation_manager->getCartItemInfo();
  if (empty($cart_item_info['order_item_id'])) {
    return;
  }

(Meaning that we probably get multiple confirmation dialogs, which would make this issue even worse?)

I'll continue tomorrow.

> I presume this would also occur with the "standard" add to cart message in commerce core, if that is not disabled.

Interestingly, that is not the case! Without "commerce_add_to_cart_confirmation" the carts get simply combined, without any message. So this is definitly related to this module.

I almost think, that this might be a paypal issue...

https://github.com/paypal/paypal-js/issues/628
https://github.com/paypal/paypal-js/issues/596

Clear steps to reproduce would definitely help to fix this issue....

Yea, I am with @jsacksick on this one. Once the user approves a payment during the PayPal checkout process, our "onApprove" js function is automatically called. This method makes an ajax call to our "CheckoutController", which invokes the "onApprove" method. This method invokes "onReturn". Where we have the code in question in place:

    if (!in_array($paypal_order['status'], ['APPROVED', 'SAVED'])) {
      throw new PaymentGatewayException(sprintf('Unexpected PayPal order status %s.', $paypal_order['status']));
    }

At this stage, we don't want any other order-state besides "APPROVED" or "SAVED". It is still unclear to me, how we can have any other state at this point. Since the whole validation starts AFTER the user approves the payment. And at this point I expect paypal to switch states from CREATED to APPROVED / SAVED.

The only reason I see, would be a race condition, where the state has not yet switched from CREATED to APPROVED / SAVED and "onApprove" has already started. But the react implementation for example also doesn't check the state before processing "onApprove" (https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187...) so I guess this is pretty safe...

Another reason could be, that the JSON sent to PayPal at the start of the transaction is broken / incorrect, but that should leave us with "PAYER_ACTION_REQUIRED" like in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active and is also very unlikely, since at the time "onApprove" fires PayPal approves the payment...

Lastly, eventually the user or some random js plugin removes the "skip_payment_creation" url query parameter, which would lead to a new payment creation at the end of the "onApprove" php callback:

    if ($flow === 'mark' && !$request->query->has('skip_payment_creation')) {
      $payment_storage = $this->entityTypeManager->getStorage('commerce_payment');
      /** @var \Drupal\commerce_payment\Entity\PaymentInterface $payment */
      $payment = $payment_storage->create([
        'state' => 'new',
        'amount' => $order->getBalance(),
        'payment_gateway' => $this->parentEntity->id(),
        'payment_method' => $payment_method->id(),
        'order_id' => $order->id(),
      ]);
      $this->createPayment($payment);
    }

But the logged example has that query parameter in place... No idea....

@jsacksick yea I think, I wanted to explicitly override the label on the selection screen only. But that's not necessary, agreed. I removed it.

Sorry for unsupporting 3.x so fast, this was reverted again.

Still 4.x should be supported. Although it's not a "REAL" major release, but more a move to SemVer versioning with a few bigger changes.

We could:

1. Add a 4.x Branch here, which supports Drupal >= 10.3
2. Simply add support for both releases (if they are really compatible with this module)
3. Drop 3.x support in the current branch as the MR says. In this case we should also align the Drupal requirement with the field_group drupal requirement (only support Drupal >= 10.3).

Ok, that should be it! Please review!

Ok, this should be it. Here are some screenshots:

We also thought about an additional optional description field, which could be displayed right under the payment method. @jsacksick should we implement this here as well, or create a follow-up issue?

Yes, this was fixed in 4.x through 🐛 Ensure visibility of invalid fields (JS) Fixed .

Alright! Adjusted accordingly!

Ok talked about this with @anybody internally. We feel like this would cover further use cases.

Done, please review!

@anybody the setting you describe for stripe is for the checkout display only, not the payment method selection pane.

Stripe always shows the logos on the payment method selection pane (even if they would have access to the plugin's settings there).

So I'd vote for a simple checkbox:
TRUE => Show logo instead of string
FALSE => Show string

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

Alright, let's use "FALSE" as the default, I don't think anyone really relied on the log message anyway.

Extra Field just released a 3.x beta release. I released https://www.drupal.org/project/extra_field_plus/releases/3.0.0-beta5 → .

Should be fixed in 4.x.

They are currently also green. Its random ajax test behavior.

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

grevil → created an issue.

LGTM! The PHPUnit failure seems to be related to an ajax tests, which seems to fail randomly. As the test succeeded, right before I added a comment, everything will be fine.
We can tackle the random phpunit failures in a follow-up issue (They also succeed locally).

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

Agreed, LGTM! Thanks everyone!

All done. !102 is not yet merged, since the tests fail again after rebase. See 📌 Make tests pass on 8.x-3.x Active .

!102 will be the last commit going into 3.x. Any further work will be done in 4.x.

Tests fail again.

Changes LGTM! I don't see a reason why we won't allow failures for phpstan and eslint.

Let's wait for the tests to pass again.

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

Is this still reproducible in latest 4.x dev?

LGTM

This seems to be a duplicate of 🐛 Field Group Label are not translating Closed: won't fix ?

Default could also be FALSE, and we create an update hook to set it TRUE for existing installations. You decide.

This should probably do the trick already.

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

That should be it. Although this could be more seen as a cleanup issue now, as dev is already d11 compatible.

https://git.drupalcode.org/project/convert_bundles/-/merge_requests/23 now only contains the changes by @misterdidi, but not the actual module info file compatibility changes.