Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
After upgrading to migrate_tools 4.1 from 4.0, I'm running the following command:
drush migrate-import d7_user --limit=100;
However I'm receiving the following error:
The "--limit" option does not accept a value.
I've tried --limit="100 items" and a couple of other things but to no avail.
I'm running Drupal 8.6.5, Drush 9.5.2.
Comment | File | Size | Author |
---|---|---|---|
#70 | 3024399-70.patch | 22.05 KB | heddn |
#70 | interdiff_68-70.txt | 12.98 KB | heddn |
Comments
Comment #2
merlin06 CreditAttribution: merlin06 commentedI can confirm that after upgrading to migrate_tools 4.1 from 4.0 and running the following command I get a similar issue:
`drush migrate:import --group=scamwatch`
Error:
The "--group" option does not accept a value.
Comment #3
edysmpme too
Comment #4
jeffwpetersen CreditAttribution: jeffwpetersen commentedI have also encountered this.
Comment #5
Alex Monaghan CreditAttribution: Alex Monaghan as a volunteer commentedSeeing the same issue with group, however, my test script has group followed by limit and don't get an error about limit.
Drush is 9.4.0
PHP 7.3 RC4
Upgraded yesterday from 8.6.4 to 8.6.7 which also updated migrate_tools & migrate_plus to 4.1
I have a large and complex database that needs to be re-structured, it takes over a day to run the full import so could really do with getting this working so I can do much smaller test chunks again. Anything I can provide to help, just shout.
Comment #6
KarenS CreditAttribution: KarenS at Lullabot commentedThere is a new Drush option added in this release --idlist. I think something about that new option has broken things.
Comment #7
Alex Monaghan CreditAttribution: Alex Monaghan as a volunteer commentedI removed 4.1 (migrate_tools & plus) and installed 4.0 and the migrate is running again on 8.6.7 as it did on 8.6.4
Comment #8
KarenS CreditAttribution: KarenS at Lullabot commentedAnother interesting clue, I tried this and get the error message:
drush migrate:import --group=drupal_7
But the following works:
drush migrate:import --group="drupal_7"
Comment #9
xurizaemonI attempted to reproduce this against 8.x-4.x-dev and found that both the --limit and --group parameters worked as expected on a test site where I installed Migrate Tools 8.x-4.x-dev. I then installed Migrate Tools 8.x-4.1 and found I was able to reproduce it.
The behaviour then seemed to flip on and off, and as far as I can tell for me it's surfacing after eg module enable/disable, then disappearing after a few Drush commands. I'm aware this is not much help as far as reproduction goes :(
Setup:
drupal/migrate_tools:dev-8.x-4.x
)First test, 8.x-4.x-dev
Repro test for limit parameter:
Correct: Source contains three rows, but limited import to a single row.
Repro test for group parameter:
Correct: Two migrations are available, group parameter limits to just that group for migrate:status command.
Repro test for migrate:import group parameter:
Correct: Only the migration from csv_source_test group was run.
Repro test for migrate:import tag parameter:
Second test, 8.x-4.1
Then I downgraded from 8.x-4.x-dev to 8.x-4.1 and was able to reproduce the reported issues.
Huh?
I can now reproduce the issue, but I can't explain how it arose after downgrading from 8.x-4.x-dev to 8.x-4.1 because they appear to be the same code? (
git diff 8.x-4.1..8.x-4.x
shows no changes.)And then moments later the behaviour vanished. Having made no further changes, I can't reproduce it any more.
Comment #10
makkus183 CreditAttribution: makkus183 as a volunteer commentedSeeing the same issue with --group:
drush migrate-import --group=top_import --update
drush migrate-import --group="top_import" --update
In both options i get:
Comment #11
robcast CreditAttribution: robcast commentedI also see the "option does not accept a value" problem with the
--feedback
option in 4.1.Sometimes it helps to put the value in quotes or remove the quotes. Sometimes it starts to happen after a
drush cr
...Comment #12
hudriComment #13
hudriSeems it is caused by update of Plus v4.1 and not by update of Tools 4.1. In my case my exisiting migrations do work with Tools v4.1 and Plus v4.0, but stop working as soon as I update Plus to v4.1
Comment #14
arnested CreditAttribution: arnested at Reload commentedThe issues seems to be in the way https://github.com/consolidation/annotated-command parses the drush command options.
Attached is a patch correcting this.
Comment #15
arnested CreditAttribution: arnested at Reload commentedComment #17
arnested CreditAttribution: arnested at Reload commentedCode style fixes to patch.
Comment #19
mpp CreditAttribution: mpp at District09 for District09 commentedI can confirm the same behavior:
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedThe latest patch looks sane to me. You have to declare your options both in the doxygen and give default values in the function signature.
Comment #21
arnested CreditAttribution: arnested at Reload commentedImproved patch added. This will hopefully parse the tests.
Comment #23
arnested CreditAttribution: arnested at Reload commentedFailed again (of course now I look at it again).
Let's try this patch., then...
Comment #24
arnested CreditAttribution: arnested at Reload commentedYay. Tests succeeded.
I'm raising the priority to 'Major' as the drush commands are not really working without this fix.
Comment #25
chaitanya.maili CreditAttribution: chaitanya.maili as a volunteer commentedI have tested the above patch, it worked fine for me.
I feel this is good to merge.
Comment #26
daveianoThe path in #23 fixes the problem for me, too.
However if the migration fails (due to errors in my own custom logic in my migration.yml), it gives a strange error message, which I first misunderstood as the previous --limit error:
I don't know if this is related, but you can see the migration fails (100 of 100 items failed), but the error message says 'Migration - 100 failed'
EDIT: Forget what I was saying, the message says how many items fail in the migration process, everything is correct.
Comment #27
kevinquillen CreditAttribution: kevinquillen at Velir commentedSame here. I can reproduce this by doing a cache clear. The first call will fail. The subsequent calls do not. Clearing the cache again will cause the error.
Here is the output:
Is there something about the cache rebuild that causes this?
Comment #28
kevinquillen CreditAttribution: kevinquillen at Velir commentedThe supplied patch fixed the group and limit issue, but introduces a new one:
The "--feedback" option requires a value.
In the patch, setting 'feedback' to FALSE corrects this behavior. I could no longer reproduce my issue, that said, I am only using the group, limit, and feedback options in my work so far.
The console output also looks correct too for the help after making that change:
migrate:import [--all] [--group GROUP] [--tag TAG] [--limit LIMIT] [--feedback] ...
The initial patch would have it as
[--feedback FEEDBACK]
and any value passed to it would cause a fatal error.I say this needs some work to ensure that the options and flags are being handled properly.
Comment #29
kevinquillen CreditAttribution: kevinquillen at Velir commentedComment #30
arnested CreditAttribution: arnested at Reload commentedBut as far as I can tell feedback should take a value
I never used the feedback option before, but when testing with the patch it appears to do what it is supposed to do
drush migrate:import --all --feedback=10
Comment #31
robcast CreditAttribution: robcast commentedI always used feedback with a value
--feedback=500
and I think this is the most useful option. I don't want to see 10000 lines of feedback for an import of 10000 rows but 10 or 20 are fine.In the past feedback without a value was the same as
--feedback=1
but I could live without that shortcut.Comment #32
kevinquillen CreditAttribution: kevinquillen at Velir commentedGot it. Didn't know feedback took a value. That makes sense now.
Comment #33
arnested CreditAttribution: arnested at Reload commentedI had a look in https://github.com/consolidation/annotated-command to see if we could specify that behaviour but it does not appear to be the case.
It might be possible to abuse
InputOption::VALUE_OPTIONAL
sincetrue
will be cast to 1, but I'm not sure going down that path is wise.Comment #34
kehan CreditAttribution: kehan commentedI think we can say that #23 does work, and feedback requires an option.
Comment #35
Alice Heaton CreditAttribution: Alice Heaton commentedSame issue, patch at #23 works for me
Comment #36
juampynr CreditAttribution: juampynr at Lullabot commentedPatch at #23 works for me too.
Comment #37
ressa CreditAttribution: ressa at Ardea commentedThanks @arnested, #23 also works for me.
Comment #38
Jorge Navarro CreditAttribution: Jorge Navarro commented#23 works
Comment #39
jcmartinez#23 Works!
Comment #40
steveoriol#23 works on D8.7.0, Drush 9.6.2 and drupal/migrate_tools:4.1.0
Comment #41
DanChadwick CreditAttribution: DanChadwick commented#23 fixed it for me too.
Comment #42
mpp CreditAttribution: mpp at District09 for District09 commentedRtbc++
Comment #43
pixlkat CreditAttribution: pixlkat commentedRTBC++ for me as well -- #23 fixed my issue.
Comment #44
MKorostoff CreditAttribution: MKorostoff commentedPatch is #23 works for me
Comment #46
heddnComment #47
mpp CreditAttribution: mpp at District09 for District09 commentedAwesome, thanks all!
Comment #49
heddnThis unfortunately broke HEAD. We'll need to figure out how to fix that. For now I've reverted.
Comment #50
andypostTesting should be extended
Comment #51
heddnThis could also use an IS update. This might not be an issue any more. I went to reproduce the issue as defined in the IS and I'm not able to do so:
Comment #52
andypostIirc it was failed to parse
idlist
passed but then it went other wayComment #53
tonytheferg CreditAttribution: tonytheferg commentedI was getting
The "--tag" option does not accept a value.
and applied #23 and it worked. @heddn, Just curious what "breaking HEAD" means?Thanks!
Comment #54
BrightBoldI also got this error with
--idlist
and the patch in #23 fixed it. Not sure why it's breaking HEAD but it seems to work locally.(FWIW, I don't think this is a problem with earlier versions of Drush. I had used
--idlist
multiple times successfully earlier this weekend under 8.x-4.1 with no problems, and then realized my Lando container had Drush pinned at 8.1, which isn't supported with Drupal 8.7, so I upgraded to Drush 9 and that's when the problem occurred.)Comment #55
heddnHere's the patch in #23 re-rolled against HEAD. It should fail tests.
Comment #57
APolitsin CreditAttribution: APolitsin commentedThe module does not work for most user cases and there is no possibility of downgrade. I think this is a critical problem.
Comment #58
ThomWilhelm CreditAttribution: ThomWilhelm commentedWhilst I agree this is a really important issue, it's not critical as per priority guidelines: https://www.drupal.org/core/issue-priority
You can apply the patch from this ticket for example, so that's a known workaround.
Comment #59
Balu ErtlComment #60
labboy0276 CreditAttribution: labboy0276 at Tandem commentedThanks for the patch, it worked. I usually always got this after a
drush cr
. My original work around was to rundrush ms
and then the commands worked ok again for some reason.Comment #61
ilaz CreditAttribution: ilaz at Balidea commentedThe "--update" option does not accept a value.
It show this error with the patch from #55.
I changed that patch with
'update' => self::REQ,
and now it works.Comment #62
heddnDid some more debugging on this today. And the int values from `self::REQ` are passed along. And those keep getting passed around. And break things. We need the actual string values from those things. So, the test failures here are legit and should be fixed.
Comment #63
heddnLet's see how this goes. It has more test coverage. And most importantly, it reproduces the bug and tests it several different ways.
Comment #64
heddnLet's add a little more test coverage.
Comment #65
heddnAnd the migrate:status command was broken. Let's fix that.
Comment #66
heddnTagging for some manual testing. It seems to work for me, but :shrug: I'd like some feedback from the community.
Comment #67
moshe weitzman CreditAttribution: moshe weitzman commentedSelf::REQ means that when the option is presented, it must be presented with a value. This is appropriate for --limit and others.
The benefit of this is that
Comment #68
heddnFixing #67
Comment #69
moshe weitzman CreditAttribution: moshe weitzman commentedPretty close. I dont see value in fiddling like
$options['group'] will be a string or NULL when you receive it. Seems like this code provides no benefit. There are lot of new isset() and I they could be just $options['foo'] instead of isset($options['foo'])
$options['all'] can only be TRUE or FALSE. You cant get passed anything else.
Comment #70
heddnThis should fix #69.
Comment #71
moshe weitzman CreditAttribution: moshe weitzman commented- * @option idlist-delimiter The delimiter for records, defaults to ':'
FYI Help shows default value automatically.
It may be helpful in the tests to pass --format=json so you have machine parseable output. At least for migrate:status command. Just a thought.
Comment #73
heddnOK, taking the feedback from #71, merged it with the previous patch and committed this thing. Thanks everyone for your feedback and persistence on seeing this land.
Comment #74
maximpodorov CreditAttribution: maximpodorov commentedWhat about the new release?
Comment #76
codebymikey CreditAttribution: codebymikey at Zodiac Media commentedThis commit breaks the
requirements
when--execute-dependencies
is specified.is not the same as
The
requirements
property is built and set in the\Drupal\migrate\Plugin\MigrationPluginManager::createInstances
function.Trying to follow the reason behind the change leads to this.
Since there's no interface method available to retrieve that property, the only viable solution is to use
MigrationInterface::getMigrationDependencies()
, and build a similar list of dependency ids.I'll create a separate issue for this bug so it's easier to manage.