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!
pacproduct → created an issue.
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.
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.
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.
pacproduct → created an issue.
pacproduct → created an issue.
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;
}
}
@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?
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?
pacproduct → created an issue.
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.
@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.
@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.
MR works great, thanks!
See attached patch a starting suggestion.
pacproduct → created an issue.
@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
(just as a side note, we're facing the same issue with module term_reference_tree → - here is their equivalent issue: Use widget for taxonomy term parent field → )
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.
@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.
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.
Yay! Tests pass :)
Switching status to "needs review".
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 :)
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
pacproduct → created an issue.
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?
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.
Oh looks like I just figured out how to push to annmarysruthy's MR, so it should be updated with my suggested changes now.
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.
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);
pacproduct → created an issue.
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? :)
Could it be related to www.drupal.org/project/drupal/issues/3230708 → ?
We do face this issue too. Is there any known workaround?
Our env: Drupal 10.0.9 & content_sync 3.0.x-dev
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?