Account created on 2 April 2010, over 14 years ago
#

Merge Requests

Recent comments

🇫🇷France pacproduct

Upgrading from Drupal 10.1 to Drupal 10.3.6 broke the markdown module as explained in this issue, and the patch in MR !29 works for me too and does solve the issue with Markdown 3.0.1. Thanks!

🇫🇷France pacproduct

Thank you @ufku for your fast reply. Your patch does 100% solve the issue we were having.

We're still facing performance issues with the browser itself, but it's way less critical and it makes sense for it to scan the entire directory in that case, so I'll investigate and if I need more help (or have a patch to suggest) I'll open a separate issue.

Thanks again! :)
Cheers.

🇫🇷France pacproduct

Hi again! :)

I configured memcache on our project as the cache backend for all bins and here is what I can observe, with "hot" caches in both cases:

  • - Time to scan my directory without memcache: 8 seconds
  • - Time to scan my directory with memcache: 3 seconds

So, using a proper cache backend does improve things a lot. In my case though, these are the times to load a single picture on the site, so it is still too slow but I think the remaining optimizations need to be done on IMCE's end (see related issue: www.drupal.org/project/imce/issues/3443768 🐛 Poor performance when downloading files from a non-standard filesystem Active )

For information, I did the test again while profiling with XDebug. Response times are a lot worse while profiling as expected, but the performance difference with/without memcache can be seen:

Without memcache (total 28 seconds):

With memcache (total 19 seconds):

----

I guess s3fs could still preload all metadata when opening a directory as a "nice to have", but considering the results above and your explanations, I think the main culprit on out project isn't s3fs but the way it gets used by IMCE + the fact that the website we're migrating has 20K files in a single directory which doesn't sound like a good practice to begin with.

Thanks a lot for your help @cmlara :)
I'll let you decide whether this issue should stay open for a future improvement of s3fs or not.
Cheers.

🇫🇷France pacproduct

Hi @cmlara and thx for your quick feedback :)

We're using the default cache backend at the moment, which is the database indeed.

When profiling what the imce module does, I end up with the following map (sorry it's in french - Columns read: Incl., Self, Distance, Call(s), Function called).

We can clearly see that the database gets hit 19K+ times (the number of files), and I believe you're right: that is via Drupal\Core\Cache\DatabaseBackend, so using another cache system is an interesting lead, I'll investigate. Thanks.

🇫🇷France pacproduct

We faced this issue while migrating a Drupal 6 site to Drupal 10.

The Drupal 6 site uses the standard local file system, but the new one has been configured to store its private files to a remote S3 server with s3fs .

I'm not sure why "realpath" is used initially when the resulting directory is only a scheme (e.g. 'private://'), neither why it's a problem to "trip up certain file API functions" as documented in \Drupal\migrate\Plugin\migrate\process\FileCopy::getDirectory().

We implemented @cmlara's suggestion in a custom Plugin that extends core class FileCopy. I don't know if it's the best approach but it works for us, so maybe it will for some other people too. Here is the source code:

namespace Drupal\your_module\Plugin\migrate\process;

use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\migrate\Plugin\migrate\process\FileCopy;

/**
 * Copies a file, in a way compatible with remote file systems.
 * 
 * This class extends the core "file_copy" plugin, but handles cases where
 * "realpath" can't work.
 * See file_copy's documentation for how to use it.
 *
 * Example:
 *
 * @code
 * process:
 *   path_to_file:
 *     plugin: remote_compatible_file_copy
 *     source:
 *       - /path/to/file.png
 *       - public://new/path/to/file.png
 * @endcode
 *
 * @see \Drupal\migrate\Plugin\MigrateProcessInterface
 *
 * @MigrateProcessPlugin(
 *   id = "remote_compatible_file_copy"
 * )
 */
class RemoteCompatibleFileCopy extends FileCopy implements ContainerFactoryPluginInterface {
  /**
   * Returns the directory component of a URI or path.
   *
   * Unlike `FileCopy::getDirectory`, if the path to the directory ends up
   * being the scheme only (e.g.: 'private://') and if `realpath` fails at
   * determining the path, we do not return FALSE but the scheme alone.
   *
   * @param string $uri
   *   The URI or path.
   *
   * @return string
   *   The directory component of the path or URI.
   */
  protected function getDirectory($uri): string {
    $dir = $this->fileSystem->dirname($uri);
    if (str_ends_with($dir, '://')) {
      $realpath = $this->fileSystem->realpath($dir);
      if (FALSE !== $realpath) {
        return $realpath;
      }
    }
    return $dir;
  }

  /**
   * Determines if the source and destination URIs represent identical paths.
   *
   * Unlike `FileCopy::isLocationUnchanged` we always return FALSE if one of
   * the given paths cannot be determined.
   *
   * @param string $source
   *   The source URI.
   * @param string $destination
   *   The destination URI.
   *
   * @return bool
   *   TRUE if the source and destination URIs refer to the same physical path,
   *   otherwise FALSE.
   */
  protected function isLocationUnchanged($source, $destination): bool {
    $sourceRealPath = $this->fileSystem->realpath($source);
    $destinationRealPath = $this->fileSystem->realpath($destination);

    if (FALSE === $sourceRealPath || FALSE === $destinationRealPath) {
      return FALSE;
    }

    return $sourceRealPath === $destinationRealPath;
  }
}
🇫🇷France pacproduct

@emixaam I'm not sure what the best approach would be.

If the issue is that the service name is weird, I guess we could:
- Rename the class and its corresponding service. That could impact some code elsewhere in the project, I did not check.
- As the services currently declared by this module don't always match the name of their corresponding classes, another approach would be to rename the service only but not the class itself (e.g. "migrate_tools.helper").

If the issue is that the source code is not well-enough organized, that would require a bit more rework but maybe we could:
-Move all these functions into a separate service, but I'm not sure how to call it: these are utility helper functions for tracking source IDs during the first phase of a migration, insuring the module knows which entries to delete. But there's already a service with a name suggesting it handles that sync task (among other things): "migrate_tools.migration_sync" (Drupal\migrate_tools\EventSubscriber\MigrationImportSync).
- Therefore, another consideration would be to completely remove class Drupal\migrate_tools\MigrateTools (which initially contained a small static function buildIdList) and put everything into the existing service "migrate_tools.migration_sync"

I feel like the decision is up to the maintainer on what the best approach for their module is at this point, but what do you guys think?

🇫🇷France pacproduct

Hi,

I feel like I'm facing this exact issue with webform version 6.2 (on Drupal 10): Honeypot's checkbox "Add time restriction to XXX webform" cannot be unchecked once it's been checked. I had to manually edit my webform's yaml configuration file to remove this option.

In Honey pot's global configurations, I do not have the "protect all forms" option checked.

Am I missing something, or should a new issue be opened?

🇫🇷France pacproduct

Because branch `6.0.x` recently changed, I had to rebase my MR and resolve conflicts. However, now the MR itself doesn't apply anymore to version `6.0.2` of this module.

So here is an attached patch specfically for version `6.0.2`, so people can keep applying it easily if needed.

🇫🇷France pacproduct

@bbu23 I believe my comment in #19 is still valid, but the best is to try it to be sure.

I feel like the global tendency in programming is to favor strict typing more and more as it tends to reduce issues and bugs.
Thus personally, I'm kind of okay with the idea of configuring what is the type of data you're expecting from the source.
Although maybe `migrate_tools` could warn you when source IDs type do not match the configured type so you know something is dubious, instead of converting it silently which causes it to rollback everything later...

Everyone's situation isn't the same, and there might be cases where having a strict source ID typing wouldn't fit one's need. From the top of my mind, the only case that could be problematic that I can think of is if the source system were to return a mix of different types as the primary ID (a mix of strings and ints, for instance). In that particular case, having a "loose type" for source IDs could be useful.
Hard for me to say which approach is better...

-- --

With regards to your case: If setting properly the source ID types on your migration configurations solves your rollback problem without patching `migrate_tools`, it feels like a quick and healthy fix in my opinion. In which case, you may apply the patch from the referenced issue if it fixes your performance problem.
Otherwise, you may have to merge the 2 patches together. Not sure how tricky this is going to be. YMMV.

🇫🇷France pacproduct

@bbu23 Could you be facing the issue referenced in https://www.drupal.org/project/migrate_tools/issues/3378047 🐛 drush migratation with --sync has suboptimal performance (v6) Active ?

More generally, I have the feeling that this thread should take into consideration the other thread I posted above, because the way migrate_tools_migrate_prepare_row currently works gets exponentially slower with time and should be fixed first, in my opinion.

🇫🇷France pacproduct

See attached patch a starting suggestion.

🇫🇷France pacproduct

@Sam152 thanks, your solution works great.

I only had to adjust slightly the source in our case (state => status), with our generated migrations from a D6 site:

process:
[...]
  moderation_state:
    plugin: static_map
    source: status
    default_value: draft
    map:
      '0': draft
      '1': published
🇫🇷France pacproduct

Facing this too.
Patch in #2 does make it work.

I'm wondering if we could not test whether the variable is an array directly though, as that's what array_diff is expecting. So we're sure it receives the right data type.

Attached patch is a suggestion with this approach + Comment explaining what the problem is.

🇫🇷France pacproduct

@MaxPah I cannot reproduce your issue. Also, migration_id cannot be used as primary key as this column doesn't contain unique entries.

We could add an arbitrary serial 'id' as unique identifier and as primary key. Although it won't be of any use, I guess it's good practice anyway.

I'll rework that shortly.

🇫🇷France pacproduct

Just added a commit.

I noticed that the migrate_tools_migrate_prepare_row hook is running during the PRE_IMPORT phase, and also during the IMPORT phase. That was duplicating all SyncID entries although we need them during the PRE_IMPORT phase. So I added a little something so IDs get recorded during the PRE_IMPORT phase only. Then, they stay available in database during the actual migration phase just in case another module would need them, and they get cleaned during the POST_IMPORT phase.

Also added some missing comments.

🇫🇷France pacproduct

Yay! Tests pass :)
Switching status to "needs review".

🇫🇷France pacproduct

I'm not sure what makes tests fail. Seems like the new database table is not found? When I install the module locally, it does get created correctly though.

What am I missing? Any help for fixing the CI would be appreciated :)

🇫🇷France pacproduct

Hi!
For info, I'm pretty sure the issue described here is the exact same one I faced. But because I use migrate_tools version 6 for Drupal 10, I created a dedicated issue. See https://www.drupal.org/project/migrate_tools/issues/3378047 🐛 drush migratation with --sync has suboptimal performance (v6) Active

🇫🇷France pacproduct

I am facing the same issue, but I'm not trying to create a custom entity myself: I wanted to use the 'ad' module which defines an 'ad_content' entity with no bundle.

I tried declaring a Scheduler plugin (with the idea to eventually suggest it as an addition to the 'ad' module), but after several tries and some xdebug sessions, I realized that the scheduler module explicitly excludes entity types that do not have any bundle (in \Drupal\scheduler\SchedulerPluginManager::findDefinitions):

    foreach ($definitions as $plugin_id => $plugin_definition) {
      [...]

      $entityType = $this->entityTypeManager->getDefinition($plugin_definition['entityType'], FALSE);
      if (!$entityType || !$entityType->getBundleEntityType()) {
        unset($definitions[$plugin_id]);
      }
    }

My Plugin definition gets unset right there.

Is there a workaround for enabling scheduling on contrib entities with no bundles?

🇫🇷France pacproduct

Hi everyone :)
I'd like to add my experience on this matter, for info.

I was facing the initial issue as described by @ion.macaria, (with Drupal 10.0.9):
TypeError: array_filter(): Argument #1 ($array) must be of type array, null given in array_filter() (line 134 of [...]/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php).

I was facing the issue when trying to add a custom block containing a media field: When clicking button "Insert selected", nothing was happening on screen, an HTTP error 500 was thrown in the console and the message above was saved in the watchdog.

Patch #2 worked around the error message, but did not solve the behavior in my case, no media was attached and the following error was displayed on screen: "No items selected."

More generally in fact, trying to add images via the dedicated page (/media/add/image) didn't work for me either, as submitting the form was raising an error message: "Image field is required." although I had selected a file. And then I noticed that I also had 413 HTTP errors just after selecting the picture file to upload:
Turns out the issue was with NGinx which wasn't configured to accept large requests, and I was trying to upload pictures larger than 2MB.

Maybe we could argue that Media should be able to handle such errors more gracefully, but (in my case at least) the issue was not directly related to Drupal in the end. YMMV.
Reconfiguring Nginx to accept larger requests solved the problem for me.

🇫🇷France pacproduct

Oh looks like I just figured out how to push to annmarysruthy's MR, so it should be updated with my suggested changes now.

🇫🇷France pacproduct

Hi, here is a patch that includes annmarysruthy's work, and adding a few more fixes needed for the module to work correctly on Drupal 10.
Attached.

🇫🇷France pacproduct

Here is a simple patch attached that extends the hardcoded "string" type so it now accepts "string" and "array".
It seems to solve the issue in my case.

As a side note, I'm not sure how the following code works out if id_token is an array, in \Drupal\openid_connect\Controller\OpenIDConnectRedirectController::redirectLogout():

              $url_options = [
                'query' => ['id_token_hint' => $this->session->retrieveIdToken()],
              ];
              [...]
              $redirect = Url::fromUri($endpoints['end_session'], $url_options)->toString(TRUE);
🇫🇷France pacproduct

I'm facing this exact same issue too, maybe this should be an option?

@rogeriocc Would you care to share how you solved it with some code examples? :)

🇫🇷France pacproduct

We do face this issue too. Is there any known workaround?

Our env: Drupal 10.0.9 & content_sync 3.0.x-dev

🇫🇷France pacproduct

Version 6.0.1 does seem to fix the issue as long as the ids' "type" property is set correctly.

In my case migrate_tools was rolling back all my items because the source ID was an integer, and I had defined:

  ids:
    source_id:
      type: string
      max_length: 100

Fixing my configuration to type "integer" fixed everything:

  ids:
    source_id:
      type: integer

I overlooked it in the first place, so I'm wondering if this subtlety is documented somewhere?

Production build 0.71.5 2024