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

fix: undefined command summary #2614

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Conversation

pwtyler
Copy link
Member

@pwtyler pwtyler commented Aug 6, 2024

Fixes undefined notice for $command_summary during WP-CLI/Drush execution
This was broken in #2589

https://github.com/pantheon-systems/terminus/pull/2589/files#diff-0940cda3af08922f69b1bf2e6f64c5c9c291135936a398e5303435a82cd234b5L81-L82

@pwtyler pwtyler requested a review from a team as a code owner August 6, 2024 17:02
@pwtyler pwtyler changed the title fix undefined command summary fix: undefined command summary Aug 6, 2024
namespacebrian
namespacebrian previously approved these changes Aug 6, 2024
@namespacebrian
Copy link
Contributor

I'm a bit confused. I see that $command_summary is no longer set after the previous PR. What confuses me is that this PR looks like it's adding a new assert to a test. i'm unfamiliar with logger->expects() and $this->once(), and whatever object with() is being called on at that point (not sure what method() returns)..

It's a new assert.. so I'm not sure how it'd be putting the command summary back, and if an error is happening because the command summary is null, i'm not sure how adding a new assert to the test would prevent that error...

@namespacebrian
Copy link
Contributor

@kporras07 would probably understand 🙃

@pwtyler
Copy link
Member Author

pwtyler commented Nov 8, 2024

I'm pretty sure I borked this pushing the wrong branch or something, I don't really recall what the test was supposed to be fore. I was looking at it yesterday because I confused myself. The issue I intended to fix is still present, I'm going to poke at this today.

@pwtyler pwtyler force-pushed the fix-undefined-command-summary branch from 97d2947 to 8bc7810 Compare November 8, 2024 20:49
@pwtyler
Copy link
Member Author

pwtyler commented Nov 8, 2024

Re-discovered the process above— I added a test and reverted the fix to see it fail, but I expected phpunit to balk at the PHP notice and it didn't.

@pwtyler pwtyler force-pushed the fix-undefined-command-summary branch from 8bc7810 to fa7e3da Compare November 8, 2024 21:41
@pwtyler
Copy link
Member Author

pwtyler commented Nov 8, 2024

Unit test I wrote doesn't actually run, removed.

@pwtyler pwtyler merged commit 10cd3e1 into 3.x Dec 3, 2024
10 checks passed
@pwtyler pwtyler deleted the fix-undefined-command-summary branch December 3, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants