Jabxjab archive:dump and archive:restore commands#5148
Jabxjab archive:dump and archive:restore commands#5148greg-1-anderson merged 214 commits into11.xfrom
Conversation
Create database.tar sub-archive; Create files.tar sub-archive; Create master archive.tar.gz
| public function getCommandList(): array | ||
| { | ||
| return $this->commandList; | ||
| return array_filter($this->commandList); |
There was a problem hiding this comment.
Do you know why it became necessary to add this? Would it be possible to prevent empty items from being added to the list, rather than remove them every time the accessor is called?
There was a problem hiding this comment.
@greg-1-anderson Spent some time investigating it and did not found a real cause of this. What I found is that the dependency references for config.export.commands and config.export.commands turining into NULL once added which causes fatal error on Drupal10 CI pipeline but not on Drupal9-related ones.
|
@weitzman I think this PR is in pretty good shape now. Could you please review again? |
| * - php | ||
| * | ||
| * @command archive:dump | ||
| * @aliases ard |
There was a problem hiding this comment.
Lets add @validate-php-extension Phar to both commands
| 'code' => false, | ||
| 'files' => false, | ||
| 'db' => false, | ||
| 'destination' => null, |
There was a problem hiding this comment.
The default value should never be null. Instead use self::REQ in most cases, indicating that a value must be provided when passing this option. Default value docs are at https://github.com/consolidation/annotated-command#option-default-values
| } | ||
|
|
||
| /** | ||
| * Returns TRUE is the site is a "web" docroot site. |
There was a problem hiding this comment.
| * Returns TRUE is the site is a "web" docroot site. | |
| * Returns TRUE if the site is a "web" docroot site. |
| dt('composer.json is found (!path), installing Composer dependencies...'), | ||
| ['!path' => $composerJsonPath] | ||
| ); | ||
| $process = new Process( |
There was a problem hiding this comment.
IMO this is out of scope. How about we give a success log message to user that they should run composer install.
* field-delete: Fix field being deleted from all bundles instead of only the requested bundle (#5158) * Updates input default options and provides destination validation. * Removes automatic composer install and adds user feedback. * Resolves phpcs feedback. * Adds composer install to test to allow Drupal installation. * Resolves code sniff. * Adds needed class. * Runs composer update to resolve security warnings. * Updates guzzle version. Co-authored-by: Dieter Holvoet <dieter.holvoet@gmail.com> Co-authored-by: Ryan Wagner <ryan.wagner@pantheon.io>
|
@weitzman Feedback addressed, let me know if there's any other issues. Thanks! |
|
I added |
| ]) | ||
| ); | ||
| $this->assertTrue(is_file(Path::join($this->restorePath, 'sut', $testFileName))); | ||
| $this->installComposerDependencies(); |
There was a problem hiding this comment.
I asked to remove composer install from restore. IMO its out of scope. Either the command should archive the vendor dir, or it shouldnt. Happy to discuss.
|
@weitzman feedback is addressed, thanks! |
weitzman
left a comment
weitzman
left a comment
There was a problem hiding this comment.
This looks ready to me. Thanks for all the hard work. I'll leave the merge honors to @greg-1-anderson.
|
Thanks everyone! |
4 participants
Merging latest from 11.x into #5048