Whoops, I previously attached an incomplete patch. Here's the correct one.
#126 works really great for me but there's an issue on the entity edit link (gear icon on hover). Steps to reproduce:
- Embed entity A
- Embed entity B
- Hover on entity A, then click on gear icon to edit entity - a new tab for editing entity A opens
- Hover on entity B, then click on gear icon to edit entity - a new tab for editing entity A opens
The expected behavior on last step is editing entity B instead.
I'm attaching a patch that tweaks #126 to fix this issue.
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.
@joachim threads are resolved - ready for review.
@joachim this is ready for review again.
Doesn't that mean migrations won't run with this?
@joachim apparently not, as per discussion on #2857104 🐛 The "null" migration destination plugin can't be used Active .
@joachim thanks for your help - this is ready for review.
bember → changed the visibility of the branch 3445944-migrate-sql-map to active.
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?
bember → changed the visibility of the branch 3445944-migrate-sql-map to hidden.
@longwave thanks for catching this. It's now fixed and since @julieelman already reviewed 6979, I'm moving this back to RTBC.
bember → changed the visibility of the branch 3425692--development-services-docs to hidden.
Rebased again and all tests pass.
I can work on this one.
Here's a draft for the helper text:
Please note that overriding a setting requires defining default values for all its sub-settings (not only the things you want to override). For example, if you want to enable render cache debug output, the following code will crash the site:
-> Bad example from ticket description
The correct procedure would be:
-> Correct example from ticket description
Could someone please weigh in? Thanks!