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

[make:webhook] Add new command for Symfony's Webhook Component #1491

Merged
merged 8 commits into from Apr 6, 2024

Conversation

maelanleborgne
Copy link
Contributor

@maelanleborgne maelanleborgne commented Mar 25, 2024

Add a maker that would help create a webhook :

  • Create a RequestParser extending AbstractRequestParser and interactively define the RequestMatcher(s) to use.
  • Create a WebHookConsumer implementing ConsumerInterface with the #[AsRemoteEventConsumer] attribute.
  • Add an entry to config/packages/webhook.yaml with the name of the webhook + the RequestParser service name + empty secret with an instruction on how to set it (as a comment).

That is an easy scaffolding command but since Webhook is a quite recent component and the documentation is pretty scarce right now, I think it would make sense to have a maker to help setting things up.

image

Regex: https://regex101.com/r/S3BWkx/1

Bumps test process timeout from 10 to 30 seconds - composer is a bit slow on Windows.

Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty nifty @maelanleborgne - a few suggestions and I think we'll be in good shape..

src/Maker/MakeWebhook.php Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Outdated Show resolved Hide resolved
src/Maker/MakeWebhook.php Show resolved Hide resolved
Comment on lines 245 to 270
$choices = array_diff($availableMatchers, $addedMatchers);
$question = new ChoiceQuestion($questionText, array_values(['<skip>'] + $choices), 0);
$matcherName = $io->askQuestion($question);
if ('<skip>' === $matcherName) {
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the same for other parts of this maker: Let's add so more spacing within the code. grouping multiple single line variable declarations is A-OK. But using that enter button between multi-line declarations, conditionals, etc... improves readability..

Suggested change
$choices = array_diff($availableMatchers, $addedMatchers);
$question = new ChoiceQuestion($questionText, array_values(['<skip>'] + $choices), 0);
$matcherName = $io->askQuestion($question);
if ('<skip>' === $matcherName) {
return null;
$choices = array_diff($availableMatchers, $addedMatchers);
$question = new ChoiceQuestion($questionText, array_values(['<skip>'] + $choices), 0);
$matcherName = $io->askQuestion($question);
if ('<skip>' === $matcherName) {
return null;

@jrushlow jrushlow changed the title Add new make:webhook command [make:webhook] Add new command for Symfony's Webhook Component Mar 27, 2024
@jrushlow jrushlow added Feature New Feature Status: Needs Work Additional work is needed labels Mar 27, 2024
@maelanleborgne
Copy link
Contributor Author

This is looking pretty nifty @maelanleborgne - a few suggestions and I think we'll be in good shape..

Thank you for the review. I'm at SFlive right now, I'll try to push the changes on my way back. Just curious about the bit on unit testing the regex : testing invalid cases seems pretty hard with the MakerTestCase, should I make the method public? Use reflection method and invoke maybe?

@jrushlow
Copy link
Collaborator

jrushlow commented Mar 28, 2024

@maelanleborgne The regex test can be added to:

class RegexTest extends TestCase

I'll have to do some minor refactoring on that class to make it "multi-purpose" (ill get a pr up and merged here shortly) - but you should be able to add another test method & dataProvider to that test class and be all set.

As to the regex itself - move the regex string to a constant like:

public const GENERATED_FILES_REGEX = '#(?:created|updated):\s(?:.*\\\\)*(.*\.[a-z]{3,4}).*(?:\\\\n)?#ui';

Then you can call that regex expression from within the class and within the test without too much trouble.

Tell the folks at SFLive I said hello!

jrushlow added a commit to jrushlow/maker-bundle that referenced this pull request Mar 28, 2024
jrushlow added a commit that referenced this pull request Mar 28, 2024
@maelanleborgne maelanleborgne force-pushed the feature/add-make-webhook branch 2 times, most recently from 22c20d4 to becc2f9 Compare April 2, 2024 07:51
@jrushlow jrushlow mentioned this pull request Apr 5, 2024
2 tasks
Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you @maelanleborgne

@jrushlow jrushlow added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Work Additional work is needed labels Apr 6, 2024
@jrushlow jrushlow merged commit 35ef9e0 into symfony:main Apr 6, 2024
7 checks passed
@maelanleborgne maelanleborgne deleted the feature/add-make-webhook branch April 10, 2024 14:32
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 23, 2024
This PR was merged into the 6.4 branch.

Discussion
----------

[Webhook] show `make:webhook`

Since `v1.58.0` (https://github.com/symfony/maker-bundle/releases/tag/v1.58.0) - MakerBundle has a `make:webhook` command (symfony/maker-bundle#1491) that generates a basic `ConsumerInterface` implementation.

- Milestone `7.1` is incorrect

Commits
-------

bf7aca1 [webhook] show `make:webhook`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants