Migrate SQL map should check it doesn't get an empty array of IDs from the source or destination

I can work on this one.

bember → changed the visibility of the branch 3445944-migrate-sql-map to hidden.

Thanks! Looks good.

Hey @joachim, the merge request is a draft one and I'm still working on it, as tests using NullDestination trigger the exception I just added.

I was thinking about skipping the error if the destination plugin is instanceof NullDestination, but I'm not sure. Do you have any thoughts?

Huh, there's a NullDestination? I just made a MR for migrate_devel with precisely that -- it was in fact how I discovered this problem, because the map and message tables go haywire.

Have a look at my MR at ✨ add a null destination Needs review -- grab the code from that to change the core NullDestination.

bember → changed the visibility of the branch 3445944-migrate-sql-map to active.

@joachim thanks for your help - this is ready for review.

@joachim this is ready for review again.

@joachim threads are resolved - ready for review.

Thanks! MR looks good.

I've asked the migrate maintainers if they're OK with adding IDs to the Null destination.

I don't think this is the right approach here. The null destination should not have IDs. Declaring one as you have here is a problem, because you are saying the import method is going to return one, and it doesn't. I see three solutions

1) Have the null destination return 'null' or something as the id for every row. I don't like this, because while there isn't supposed to be a requirement that destination IDs are unique, there are monsters in the code that rather expect it, particularly around rollbacks and map source lookups by destination id. Not things you might commonly use with the null destination, but it gives me pause.

2) Have the SQL map require the destination plugins to have at least one id. Much like you've done here, but error it out as more of a requirements check, i.e. "the SQL map is not compatible with destinations that do not provide at least one id. Make a new nullIdMap that doesn't store anything and is compatible with the null destination, fix tests around the null destination to use that. Add a BC layer, deprecation, etc.

3) fix the monsters in the SQL map around having no destinatiion IDs. Store the source IDs, refuse to look up sources by destination id. If there is code in drush or rollbacks that has a problem with it outside of what we could fix in the map itself, investigate and find solutions.

I would need to investigate a bit more before I could be certain which of those is best. I'd need to look through any existing null destination tests, and other tests to see exactly what breaks, and what makes sense to have happen in those scenarios, but I do think we need to support destinations that have no IDs to the extent we can. They should be able to work for imports, even updates if it knows what it's doing.

From the issue summary:

If the Sql map gets an empty array from MigrateDestinationInterface::getIds(), because of a configuration error in a migration, say, then bad things happen.

In short: if you make a mistake, then bad things happen, Well, that is sort of expected. What we should do in this situation is catch the mistake early and give a helpful error message. It is not clear to me why you want to fix the problem by throwing an exception from ensureTables().

Can you give more information about the sort of "configuration error" you have in mind? It would help to have explicit "steps to reproduce". Maybe then we can figure out a better place to add a check.

> In short: if you make a mistake, then bad things happen, Well, that is sort of expected. What we should do in this situation is catch the mistake early and give a helpful error message. It is not clear to me why you want to fix the problem by throwing an exception from ensureTables().

Because throwing an exception from ensureTables() is catching the mistake and giving a helpful error message!

> Can you give more information about the sort of "configuration error" you have in mind?

IIRC I was using an XML source and I hadn't set in my migration yml which bits of XML data were keys. Therefore the source plugin returned an empty array of keys.

> 3) fix the monsters in the SQL map around having no destinatiion IDs. Store the source IDs, refuse to look up sources by destination id. If there is code in drush or rollbacks that has a problem with it outside of what we could fix in the map itself, investigate and find solutions.

That might be the most streamlined fix.

However, in terms of DX it's best to be told that your source or destination are currently incorrectly configured.

I don't think this is the right approach here. The null destination should not have IDs. Declaring one as you have here is a problem, because you are saying the import method is going to return one, and it doesn't.

I think it depends on the outcome of 🐛 The "null" migration destination plugin can't be used Active , which challenged the current premise of empty destinations being strictly used for tests (given requirements_met = false). In other words, if NullDestination evolves out of the testing scope, then I agree that this approach should be revised. If, however, NullDestination's purpose is indeed mocking/emulating stuff, then the added ID sounds perfectly reasonable.

Because throwing an exception from ensureTables() is catching the mistake and giving a helpful error message!

Not to mention that catching the issue earlier prevents creating a bad/useless db table.

Sorry, I did not mean to change the status in #15.

In that comment, I wrote,

Can you give more information about the sort of "configuration error" you have in mind? It would help to have explicit "steps to reproduce". Maybe then we can figure out a better place to add a check.

Comment #17 gave some more information. That might be enough to come up with a working example, and then an automated test.

The existing test coverage in the MR just confirms that we throw an exception, now that we are adding a throw statement in ensureTables(). A more useful test would mock the sort of mistake described in #17 and show how the new throw improves the DX.