Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI-1121: Fix sql imports #1581

Closed
wants to merge 5 commits into from
Closed

Conversation

danepowell
Copy link
Contributor

@danepowell danepowell commented Aug 22, 2023

Motivation

Drush SQL imports can fail if the SQL file is larger than roughly 64k. For example: acli drush site.env -- sqlc < big-sql-dump.sql

This is due to an upstream Symfony bug: symfony/symfony#21580

Proposed changes

Implement the same workaround as consolidation/site-process#74 and https://github.com/acquia/mc-cs-kafka-consumer/commit/acca146d7736f80add46c9ba5864a0b53758dc71

I'm not confident this is a good workaround. While it fixes this problem, it may introduce others (as evidenced by the failing tests). For instance, acli drush site.env -- sql-dump produces a zero-byte export. Which is weird, because without the redirect it does print to stdout as expected.

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b0457a7) 91.84% compared to head (8a40257) 91.75%.

❗ Current head 8a40257 differs from pull request most recent head ce0e236. Consider uploading reports for the commit ce0e236 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1581      +/-   ##
============================================
- Coverage     91.84%   91.75%   -0.10%     
- Complexity     1819     1822       +3     
============================================
  Files           124      124              
  Lines          6514     6522       +8     
============================================
+ Hits           5983     5984       +1     
- Misses          531      538       +7     
Files Coverage Δ
src/Helpers/LocalMachineHelper.php 64.90% <93.75%> (+0.56%) ⬆️

... and 42 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danepowell
Copy link
Contributor Author

The latest commit helps but breaks other things, including checklists (in push:artifact, for instance).

@danepowell
Copy link
Contributor Author

I just realized, it may not be this simple. I think this actually needs to be fixed in Drush since ACLI is calling Drush. We'd only need to "fix" this in ACLI if some other process was calling ACLI and experiencing the bug.

May need to get traction on consolidation/site-process#74

@danepowell
Copy link
Contributor Author

danepowell commented Nov 6, 2023

Now I understand why this appeared to work initially. The replacement error handler was swallowing the pipe error, and the lack of a callback was suppressing the SQL import error. But in fact, nothing was ever fixed here. I think this needs to be fixed upstream in Symfony and/or site-process and/or Drush first.

@danepowell danepowell closed this Nov 6, 2023
@danepowell danepowell deleted the CLI-1121 branch September 20, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant