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

Added missing getter for sales channel id to customer login event #175

Closed
wants to merge 1 commit into from

Conversation

JoshuaBehrens
Copy link
Contributor

1. Why is this change necessary?

If you want to react to this event and depend on the sales channel then you need the sales channel id in the event but can't read it.

2. What does this change do, exactly?

Adds a getter for this private field.

3. Describe each step to reproduce the issue or behaviour.

  1. Subscribe event
  2. Use sales channel in further steps
  3. Can't acquire sales channel id from event although it is passed in its constructor

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

5. Quickfix

private function getSalesChannelIdFromEvent(CustomerLoginEvent $event): string
{
    $getter = Closure::bind(function (CustomerLoginEvent $instance): string {
        return $instance->{'salesChannelId'};
    }, null, $event);
    return $getter($event);
}

@keulinho
Copy link
Contributor

keulinho commented Oct 7, 2019

Hey @JoshuaBehrens,

you are now able get the SalesChannel from the customer login event, so your change is not necessary anymore.

Nevertheless thanks for your contribution 💙

@keulinho keulinho closed this Oct 7, 2019
@JoshuaBehrens JoshuaBehrens deleted the patch-1 branch October 7, 2019 15:23
@JoshuaBehrens
Copy link
Contributor Author

The given implementation suits me. No anger 💙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants