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

Configurable Github/Gitea urls #163

Closed
wants to merge 1 commit into from
Closed

Configurable Github/Gitea urls #163

wants to merge 1 commit into from

Conversation

gerdemann
Copy link
Contributor

No description provided.

@@ -25,10 +25,18 @@

class GithubWebhook extends AbstractWebhook
{
public function __construct(Manager $manager, EventDispatcherInterface $dispatcher, ?string $secret = null)
/** @var array */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please define array shape

public function __construct(
Manager $manager, EventDispatcherInterface $dispatcher,
?string $secret = null,
?array $sourceUrls = ['git_url', 'ssh_url', 'clone_url', 'svn_url']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can You set this default value directly to property?

{
parent::__construct($manager, $dispatcher);
$this->secret = $secret;
$this->sourceUrls = $sourceUrls;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use setter for this property as it is optional and set by default.

@gerdemann
Copy link
Contributor Author

@ramunasd Can you look at it again, please? Is that what you meant?

@gerdemann gerdemann requested a review from ramunasd November 11, 2021 13:45
@ramunasd
Copy link
Collaborator

The main point was to move out $sourceUrls from __constructor(), because this parameter is optional. Please set default value directly to class property. Setter mut be called from DI, not constructor.

@gerdemann
Copy link
Contributor Author

The main point was to move out $sourceUrls from __constructor(), because this parameter is optional. Please set default value directly to class property. Setter mut be called from DI, not constructor.

I have orientated myself on the secret property. Unfortunately, I'm not that deep in the project. Can you describe what you mean in more detail?

@ramunasd
Copy link
Collaborator

I think I was quite clear :)
Nevermind, I will fix this later.

@gerdemann
Copy link
Contributor Author

Thank you very much.
I had also orientated myself on the Gitlab webhook, where it was done in the same way.

@ramunasd ramunasd changed the title feat: Make Github/Gitea urls configurable Configurable Github/Gitea urls Nov 17, 2021
@ramunasd
Copy link
Collaborator

Rebased and merged from other branch.

@ramunasd ramunasd closed this Nov 17, 2021
@ramunasd ramunasd added this to the 3.4 milestone Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants