🇭🇺Hungary @djg_tram

Temporary: Drupal\typed_data\PlaceholderResolver, line 181 (first line of scan()):

if (!is_string($text)) return [];

But this is just strictly temporary workaround, has to be fixed properly.

The first idea was to provide a language but then you receive another error. The developers should fix this...

djg_tram → created an issue.

The current versions were developed primarily on D11/C3 but also checked for compatibility with D10/C2.

1.0.3 published.

Pushed it to dev as well.

Is it really possible to not get a valid order here? It sounds a bit strange... Anyway, I would suggest to leave the parent call even then and use

    $order = self::getOrder($form_state);
    if ($order && $order->hasField('shipments')) {
      foreach ($order->get('shipments') as $shipment_item) {
        /** @var \Drupal\commerce_shipping\Entity\ShipmentInterface $shipment */
        $shipment = $shipment_item->entity;
        $plugin = $shipment->getShippingMethod()->getPlugin();
        if ($plugin && str_contains($plugin->getPluginId(), 'pickup')) {
          return FALSE;
        }
      }
    }

Oops, I see, you wrote the same. :-)

The first, quick idea is to change

$profile_name = $pane_form['pickup_profile'] ? 'pickup_profile' : 'shipping_profile';

to

$profile_name = isset($pane_form['pickup_profile']) ? 'pickup_profile' : 'shipping_profile';

Could you quickly try this?

@alezu Same here, probably, but I do havel a label() function for my entities. Why would I need an extra key?

OK. But, in https://git.drupalcode.org/project/commerce_shipping_pickup_api/-/blob/1..., the base validateForm() is left completely empty, on purpose — I didn't make it abstract like the other two functions, just so that you don't have to provide it, staying optional. So, even if it is triggered, it should do nothing in your case.

And the previous location, https://git.drupalcode.org/project/commerce_shipping_pickup_api/-/blob/1..., calls the parent function all right. So, right now, I'm still not sure what causes the trouble for you. :-)

The whole reason for the validation change was that I'm working on https://www.drupal.org/project/commerce_shipping_pickup_brt → and it required modifications to the framework as well. Basically, this provider doesn't return the whole list of pickup points (or shows them all on a map) but requires us to send in a base address first and only returns a set number of pickup points around that address. This means that the submodule has to ask the user for an approximate address first (and, of course, this can be different, doesn't have to be the billing address of the customer).

I already had the submodule mostly working when, during testing, we found out that various scenarios lead to problems. There are two fields that accept the name and address of the selected pickup point. However, there's a problem: at first, it's obvious to set these fields as #required — if the point isn't selected, this fields are empty, so this stops the user from going forward without a pickup point. However, and this is the problem: if the user has a previously stored address but decides to enter a new one right there, Drupal will not Ajax refresh the pane because the required fields are empty. Contradiction.

Hence the validation: I tried to make it possible not to rely on the usual Drupal Commerce auto-validation but to make it possible for any submodule to provide its own validation. If you look into them, eg. https://git.drupalcode.org/project/commerce_shipping_pickup_brt/-/blob/1..., yes, they have their own that doesn't rely on #required attributes any more.

Obviously, it's still problematic but your experience and input comes in handy because, just with this BRT, we're right now experiencing different behavior in two different test sites, and whether it already works correct or not, both should behave at least the same way and they are not, so we're scratching our heads right now. You could provide you rown validation as well but actually, this shouldn't be required, I tried to make sure that this new validation is optional but in no way required, and if you don't need anything special like the BRT submodule, you shouldn't need to worry about it.

So, to cut it short, if you could just look briefly into your module and see what causes it to miss the validation, maybe to dpm() the values in a few spots, especially around https://git.drupalcode.org/project/commerce_shipping_pickup_api/-/blob/1... to see whether what's there in the comments is alos valid in your particular case, that would really help to sort this out.

Then could you make just one more check with rewriting this condition to:

if ($this->shippingMethod != NULL && $this->shippingMethod instanceof PickupShippingInterface) {
// - NULL when going on - validate selection
// - FALSE when changing address - don't validate, selection is unimportant

The new $this->shippingMethod->validateForm obviously cannot be called for other methods but this gets called, apparently, not just with our own methods. I suspect this would solve the issue and then I upload the fix.

The validation routine is in fact new but it should still receive a PickupShippingInterface. Do you use one of the published submodules or maybe you created your own? In the second case, could you peek into

https://git.drupalcode.org/project/commerce_shipping_pickup_api/-/blob/1...

to see what you get there that makes the assert complain?

djg_tram → created an issue.

Sorry, I spoke too soon. "To" does not but "Cc/Bcc" already does. Then only a very small suggestion to mention this in a description below the field. :-)

djg_tram → created an issue.

And the opposite is also missing, am I right? I mean, there's no way to go back to the submission from the usual Commerce order list, or when viewing/editing a particular order.

Yes, but there will be more changes, even with these lines — I hope without regression. :-)

Isn't this the same as https://www.drupal.org/project/commerce_shipping_pickup_api/issues/3406396 🐛 Dropdown or map not showing when default shipping method Active ?

As I also mentioned in another issue, I was working on some bigger changes because I'm involved with a dependent module that also required adaptation the the framework. That seems to have settled by now, so I'll upload the current state as a dev module, I'll not yet really ready to publish is before I get feedback. But this also contains the fix for the first appearance.

Good catch. The module itself is already compatible with D11 and C3 but I missed this.

There will be more changes in the near future, I'll upload the fix then.

Drupal 11 is already supported.

I can't seem to find a merge request that I could accept...

Besides, actually, I have quite a few modifications in my local copy because I needed to add a few things in a site I actively work on. But I'm not really sure each of those is general enough to be included. maybe if I only uploaded them to a dev version...?

@akolahi, @adpo If you're still interested, I added a module to the dev version.

As I moved on to Drupal 11 with the modules, I'm contemplating this. Do you think a complete address provided, just like with the normal delivery? Address, country, city, post code?

Drupal 11 supported.

Drupal 11 supported.

Drupal 11 supported.

Drupal 11 supported.

Drupal 11 supported

Fixed (https://git.drupalcode.org/project/commerce_shipping_pickup_api/-/commit...)

I experienced this in a brand new test setup. Looking into it.

@jsacksick Could you not just introduce a breaking change that kills any contrib module that depends on yours dead in its tracks? :-(

You could have been added a new dependency instead of changing it, for instance...

I hope I didn't screw up, I'm much less familiar with this Drupal Git than with GitHub. :-))

I was hoping for a git revert to function but it doesn't. :-) I'll start all over again.

Yes, I actually added that too because I created it in the meantime but feel free to pick.

My actual use case is to issue a redirect to a specific page when the user changes the country (practically to go to the starting page of that country rather than staying on the same page but with a different country setting. I use a correct middleware redirect approach so it doesn't get executed immediately but gets stored in the kernel and is executed when the current request actually finishes.

But, actually, I can achieve the same effect with using a different Country Switcher, a modified copy of yours that creates the URLs according to my specific needs. And this probably is a cleaner approach, anyway.

First, quick attempt but it seems to work for me now. Could you take a look before I create a MR?

djg_tram → created an issue.

Here we go again... Four years later (#10) and mysqli still required...

D9 actually requires PDO.

First, I would also add this file:

commerce_combine_carts.settings:
  title: 'Combine Carts Settings'
  description: 'Combining different order types.'
  route_name: commerce_combine_carts.settings
  parent: 'commerce_order.configuration'
  weight: 999

as commerce_combine_carts.links.menu.yml. This makes the settings form appear as a link in the normal Commerce settings dashboard.

Second, although I found the carts tobe combined, I'm not sure the shipment methods are handled all right. If the two carts have different shipping methods and rates — and they probably have, that's the main reason for them in the first place — how is that supposed to be handled?

Could you look into it and decide what causes it? I left Admin Toolbar as soon as the new navigation bar appeared in core (even if it's still considered experimental). I like it much better...

I never looked into the code that generates these form elements, so I really don't know. This is just a shot in the dark, but maybe a class of form-indent-level-N would be beneficial.

I can't find anything in the classes that would help us tell apart this case, so it would be a task for the core to add something that we can rely on.

I would, frankly, expect Devel to style their form to indent the dependent fieldset in this particular case.

I'm not sure why this would be a problem: the primary purpose is to remove extra space.

You seem to mix two things here. This specific toolset is, in fact, a dependent fieldset that opens up when the upper checkbox is checked. But this is not a general attributes of fieldsets, this is something specific to this Devel form. If we changed all fieldsets the way you suggest, they would become larger anywhere in the UI, not just in this dependent position.

You're welcome. Although I wouldn't call that linked documentation very clear, it's still much easier than the USB quagmire I'm currently in in another project. Not related, anyway. ;-))

I seem to remember that D.o promised to make this contribution setting automatic some time ago, sad to see it's still not working...

Sure, with code modules where things can go wrong. But with a theme?

OK, I'll look into it, my main problem is that I use GitHub almost daily for Flutter development and know it quite well while with the Drupal system, including their only half-baked solution with GitLab and the old, tedious manual publishing of new versions, I always need to consult my notes to remember how it worked the last time around. :-)

Do you think this is important? The tag is 2.x and I got the upgrade in my website all right, so it seems just fine to me as is. Is it really important to keep the branch name in sync?

I'd give it almost zero chance of not being compatible with 11 as is but could someone with an actual 11 check it out, merely by adding ^11 to the info.yml?

The patch we suggested seven years ago (!), simply casting to array, was already capable of accomplishing this. :-)

I have long moved on since and no longer bitten by this problem but this one is really ridiculous, I have to say. A very straightforward modification that obviously doesn't even require testing, it cannot cause any problems or regresssion by definition, and nobody ever bothered to accept it (or even argue against it). Must be one of the record setters in Drupalland. :-)

When I looked around, I found it discussed in some places that this is deliberate. GitLab, where the source code is hosted, doesn't allow a tag to be removed if it is still referenced, and you have no rights to remove the D.o reference as a mere developer. The D.o team could do it upon request but the old pointers to where we could ask for it are no longer valid.

Sorry, it was an error and the system doesn't allow it to be revoked. I erroneously created a 2.0 tag after 1.9, without giving much thought, and only realized the error later. I deactivated it as much as the Drupal UI allows it but I can't completely remove it.

Yes, works as designed. The reason for it is the Roles.php filter that explicitly removes the authenticated user ID from the list.

The solution is to use a grouped filter.

Set Grouping 1 to Barefoot user and the operator to Only has the 'authenticated user' role. Set all the other groupings to your actual roles, that is, Grouping 2 will be named Role 1, operator is one of your Role 1. Grouping 3 will be named Role 2, operator is one of your Role 2, and so on.

This way you will have a combobox with all your roles plus a simple user at the front.

That's still a critical bug. You have everly right to turn on a module and not starting to use it immediately. A WSOD is hardly acceptable...

Could we urge acceptance of the MR? A WSOD is, after all, quite serious a problem...

I'm not sure how this would be any different than creating a simple, stock flat rate shipping method with a fee of zero...

I never needed to do such a syncing actually, so I wrote this specific part according to the many samples on the net. If you really think this causes no problems... :-)

This isn't the proper solution because first, you don't know whether the site has a default type at all, second, you don't know whether the admin wants to apply the possibility to set a price override to this type, anyway.

A solution would be to remove this field altogether and to instruct the user to create a field with the same machine name on any product type they want to apply the opportunity to.

djg_tram → created an issue.

No, there was a WSOD that blocked everything. I could only solve it by temporarily modifying the source code (as described above). As I do module development actively myself (both some contrib and many for in-house solutions), it was possible but not all admins are created equal. :-)

It depends on the particular site and situation. If I'm already inside a site with smaller traffic volume and requirements (not one that postulates fully separated test and production environments, just the smaller garden variety ones), logged in and just see that a couple of modules need to be upgraded, it's usually much simpler to let it happen there with the UI and then run the update as well. I also use the new Navigation module with my own additions that make both db update and cache clear readily available. In these cases, I only fire up the console access with Drush if necessary (core upgrade needed or WSOD).

With more complex schemes I might go the Composer and Drush route immediately. I don't particulary like the Composer approach for modules, so I tend to avoid it with my own modules but that's more like a question of preferences. At any rate, even if many modules do recommend Composer, I still think that as long as Drupal actually has the old way, that should also work.

Especially as, unfortunately, it's still not always easy to set up an environment that provides full Drush/Composer functionality, I just happened on a site where drush cr works but drush updb does not because proc_open is disabled and while they offer up a few items of php.ini to customize, this isn't among those. And equally unfortunately, just telling the client to move to a better provider is not always the most feasible solution. :-)

I had to:

1. manually remove the Renderer argument from the constructor.
2. Clear cache so that the site will return from WSOD.
3. Re-insert the argument.
4. Run UPDB manually.
5. Then it works.

Not fun. The upgrade process should be tested a bit more, I'm afraid. :-) This was on a production site that isn't used yet intensively. It would be a major PITA on a real production site.

No, a simple cache clear doesn't help.

Same error with updating to 2.0.4, simple cache clear doesn't help.

Yes, that's the possibility but then you probably would want a new setting for the field? Even if the geo module is present, the admin might not want to use it or not for all price fields.

This was a poorly thought out issue for me to have opened.

Not necessarily. :-) Now that I looked into the code, it's not really much. It looks for either of the two dependent geo-ip packages and if present, asks them for the country. Really nothing duplicated, just a single call in each case. And then commerceguys/intl seems to maintain a country > currency map that could be queried to get the currency. All that could be placed into a very simple submodule providing this optional resolver. I'm not sure I'll have time to play with this for a week or two, and I also have something else I promised in another issue to look into but if you feel like experimenting with it, sure. Or wait a little and I'll try to do it a bit later.

This module clearly states in the README that it doesn't do any resolving on its own but uses resolvers found in the system. Which is, in my opinion, the correct approach because it separates the actual multi-currency support from a selection of additional behaviors whose requirements might be different from site to site. I was well aware of that module before I started working on mine and, frankly, this was one of the reasons, I needed a cleaner solution that makes the multi-currency handling as automatic as possible — and I thought that others might benefit from it, too.

I added explicit support for Commerce Exchanger because it's a very common requirement (I use it, too). Other resolvers can be added in a similar way if it's not possible to rely on them automatically, without any extra support, just as part of the resolver chain.

I'll try to find some time to add the conditional handling in the coming days...

I don't see what benefit hook_install would provide in this case. It's really only about a single try-catch, nothing else.

No, that's not how it works. If you use Docker, for instance, it's your responsibility to add dependent packages each time the container is rebuilt. I usually do it by adding an .sh script in an init folder, then referencing from the docker-compose.override.yml as

    volumes:
      - codebase:/var/www/html
      - ./init/30-composer.sh:/docker-entrypoint-init.d/30-composer.sh

The actual script can call the usual composer require calls but if you want to make sure you only call them unless it's already installed, you can use:

composer show | grep -q foo/bar || composer require foo/bar

Most definitively not solved and hardly as designed. It succeeded in making my site completely unusable after installation (fortunately not a production site but still). Error message as above. Cache refresh goes without issues. DB update say nothing to update. I can't remove it because drush pmu cannot find the 'secret' and 'confidential' fields in the 'consumers' table. You cannot add those manually because the schema will refresh them, of course. Complete checkmate. I'll find a way out somehow but you can be dead sure I'll never revisit the module. :-)

(Addendum, .install should warn about the required packages. You should never rely on everybody using composer to install. I looked into composer.json first because I was cautious but if a module breaks without its required packages then do you homework to make it clear before installation. And document it clearly.)

Requires "league/oauth2-server" and some other packages. See composer.json.

Those checkboxes, unless I'm mistaken, only work when accepting a PR, don't they?

I'm active in a couple of other dev communities where this is handled automatically, yes. It isn't advantageous at all that D.o made it that convoluted, also including how to make a new release. But in one point you're not mistaken, I'd consider it totally unnecessary to create PRs for my own changes and do the whole routine in an even more convoluted way.

I certainly doesn't want to conceal contributions, quite the opposite, I'm grateful for them. I could've accepted the PR in the first place but frankly, both changes were so minor that, given the extra complexities of the D.o process in general, this seemed much more efficient. I never intended to create animosity, so I apologize if someone feels so, but I'm not sure I can change those boxes now after the fact.

That's true and a good catch, I created it on a site where shipping was present and I never actually realized that this is an extra dependency. We have to look into how we can make it optional. Actually, none of processors mentioned there is essential. If you look into the code, they're purely there to put their calls inside a try-catch block. The stock Commerce merely throws a `CurrencyMismatchException`, resulting in a WSOD, if the cart has items in more than one currency, I only wanted to avoid that and show a regular, user friendly error message instead of the WSOD. So, in your particular case, you should be able to simply comment out `commerce_currencies.safe_early_order_processor` and `commerce_currencies.safe_late_order_processor` without any further issue.

I never decorated an existing class conditionally. Now that I googled for it, maybe https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio... → would be the solution?

Both the commit and the actual changelog of the new version refer to this issue. I'm not sure D.o allows us anything more than that but do tell me if I missed something.

Thanks, I had other a few other changes lined up, so I added it directly instead of the PR.

I'm not sure that would be enough. I added the JS, the arrows appear but the functionality is still missing. What's your experience, does it work for you?

Could it be related that I seem to get tons of "Theme hook se_twitter not found." warnings in the log recently? Looking into the code, it's "se_x" now, of course. Yes, an older site, before Twitter was renamed.