Account created on 4 June 2010, over 15 years ago
  • Drupal developer at Liip 
#

Merge Requests

More

Recent comments

🇭🇺Hungary czigor

Added some CS and test fixes as well.

🇭🇺Hungary czigor

@milanbombschliip I've already started, thanks. And now the actual review part.

🇭🇺Hungary czigor

Finally hopefully I have some time to review this properly.

On the one hand https://github.com/ddev/ddev-drupal-contrib/blob/main/README.md says

Commit the changes in the .ddev folder after this plugin installs. This saves other users from having to install this integration.

However, https://www.drupal.org/project/search_api/issues/3520659#comment-16149749 Enabled local development env with DDEV Active says

Hi, DDEV maintainer here. I don't usually encourage committing .ddev in contexts like this.

I also feel bad about committing it, so let's remove all ide and ddev related additions from the MR.

🇭🇺Hungary czigor

Just in case someone finds this useful. I put the client id and secret into config (since that's easy to override by environment) and used the following hook:

/**
 * Implements hook_ENTITY_TYPE_load().
 */
function mymodule_consumer_load($entities): void {
  /** @var \Drupal\consumers\Entity\ConsumerInterface $consumer */
  foreach ($entities as $consumer) {
    if ($consumer->uuid() !== SomeClass::UUID_CONSTANT) {
      continue;
    }
    
    $config = \Drupal::config('mymodule.settings');
    if ($config->get('oauth_client_id')) {
      $consumer->set('client_id', $config->get('oauth_client_id'));
    }
    if ($config->get('oauth_client_secret')) {
      $hash = \Drupal::service('password')->hash(trim($config->get('oauth_client_secret')));
      $consumer->set('secret', $hash);
    }
  }
}
🇭🇺Hungary czigor

Let's use class_exists() instead.

🇭🇺Hungary czigor

We could definitely add something like WHERE collection='entity.definitions.installed' AND name LIKE %.field_storage_definitions'.

My bigger concern is if this is too database specific or not. We might want to do a SELECT, do the replacement in php and do a separate UPDATE instead.

🇭🇺Hungary czigor

To sum up our findings with @siegrist:

Since this commit commerce's BundleFieldDefinition got deprecated in favor of the implementation of the entity module. This happened 7 years ago.

The issue is that the sites that existed before the deprecation might still have commerce's BundleFieldDefinition serialized in their field storage definition. See e.g. select * from key_value where collection='entity.definitions.installed' and name='commerce_payment_method.entity_type'; So when updating to DC 3.x these definitions start referring to a nonexistent class.

This affects pretty much all payment method types, profile types and product types as well.

@jsacksick Would it make sense for you to have an update hook in commerce core 2.x that updates these field storage definitions to use the entity module implementation instead? (Once you update to 3.x it's too late.) I haven't tried but just saving the entity type might recreate the field storage definitions as well.

🇭🇺Hungary czigor

czigor made their first commit to this issue’s fork.

🇭🇺Hungary czigor

This one is fixing 09be1f63ff32bbc154f74c7367cb18023f814113 to keep the speed gain. Probably this is the better approach.

🇭🇺Hungary czigor

That's partially reverting 09be1f63ff32bbc154f74c7367cb18023f814113.

🇭🇺Hungary czigor

Patch #2 makes sense and fixes the issue for me as well.

🇭🇺Hungary czigor

I just checked the site again and now the bug is gone. I've rebooted my laptop since the last try.

🇭🇺Hungary czigor

What I've found so far is that something goes wrong during the plugin annotation parsing.
When language-url is not found the following happens:

  1. When running drush cr the annotation gets parsed in Drupal\Component\Annotation\Doctrine::parse(). This time the "[#" string gets a correct T_ATTRIBUTE (351) code which makes the annotation be parsed. Right after this the cache_discovery table contains the correct data for the language_negotiation_plugins cid.
  2. After this the cache_discovery table is emptied due to clearing the caches).
  3. After this when I go to any page the plugin definitions are rebuilt. This time the "[#" string gets a T_COMMENT (387) code which makes the annotation be skipped. The cache entry will be an empty array.

Not sure yet why the difference is.

🇭🇺Hungary czigor

Experiencing this issue. I don't know what triggered it but now I can't get rid of it. The missing plugins it's complaining about is either language-url or block_page.

🇭🇺Hungary czigor

They also offered that they could make name fields visible on the credit card form on their side. Because I have some concerns that the name we have in Drupal might not actually be the name on the credit card and it might be worse to have the wrong name as opposed to no name?

Yes, these two names might not necessarily coincide although at least in Hungary we have some rule that they should. (I.e. I cannot pay with my personal card if I ask for an invoice for some company.)

🇭🇺Hungary czigor

@Berdir Thanks for the review! Fixed the issues. Since VISA will stop working without 3DS, I think some boilerplate 3DS handling has its place in contrib.

🇭🇺Hungary czigor

Adding

    $this->userStorage->resetCache();

before

    $account = $this->userStorage->load(1);
    $account->init = $account->mail = $account_values['mail'];

in \Drupal\Core\Installer\Form\SiteConfigureForm::submitForm() makes the issue go away.

Wondering whether this is a Drupal core issue.

🇭🇺Hungary czigor

We see this issue as well when running our tests on a site and it breaks our CI. However, I was not able to reproduce this on a minimal drupal install (where only commerce_payment_example was enabled).

@omarlopesino Do you know what the other modules or config is required to see this?

🇭🇺Hungary czigor

Let's remove the custom order item fields requirement.

Production build 0.71.5 2024