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
Jabxjab archive:dump and archive:restore commands #5148
Conversation
Create database.tar sub-archive; Create files.tar sub-archive; Create master archive.tar.gz
@@ -17,6 +17,6 @@ public function addCommandReference($command): void | |||
|
|||
public function getCommandList(): array | |||
{ | |||
return $this->commandList; | |||
return array_filter($this->commandList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add @validate-php-extension Phar
to both commands
'code' => false, | ||
'files' => false, | ||
'db' => false, | ||
'destination' => null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ready to me. Thanks for all the hard work. I'll leave the merge honors to @greg-1-anderson.
Thanks everyone! |
Merging latest from 11.x into #5048