Skip to content

Split listener from ConfiguredClientsStrategy #388

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 21, 2020
Merged

Split listener from ConfiguredClientsStrategy #388

merged 1 commit into from
Oct 21, 2020

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Oct 14, 2020 •
edited
Loading

Q A
Bug fix? yes
New feature? no
BC breaks? technically speaking yes (but I would assume this class should be considered internal?)
Deprecations? no
Related tickets -
Documentation -
License MIT

What's in this PR?

This splits the listener from the ConfiguredClientsStrategy.

Why?

The listener only executes a static call. This avoids initializing the dependencies (including HttpClient) on each request/command call.

Checklist

Not sure if any of these checks should be done for this PR?

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

dbu
dbu approved these changes Oct 14, 2020
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

i think this makes sense, and people should not be using these classes.

i would still do a changelog entry to mention why we do this (avoid instantiating the strategy on every request). the changelog will also help if somebody hacked weird things in the service definition or such and wonders why things stopped working.

src/Discovery/ConfiguredClientsStrategy.php
\class_alias(Event::class, 'Http\HttplugBundle\Discovery\ConfiguredClientsStrategyEventClass');
} else {
\class_alias(LegacyEvent::class, 'Http\HttplugBundle\Discovery\ConfiguredClientsStrategyEventClass');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this was unused, correct?

Copy link
Contributor Author

@wouterj wouterj Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I've removed the ConfiguredClientsStrategyEventClass $e argument from the onEvent() method. The event was not used, so I figured the easiest way was to remove this "bc hack".

@wouterj
Split listener from ConfiguredClientsStrategy
c46491d 
The listener only executes a static call. This avoids initializing the
HttpClient on each request/command call.
@wouterj wouterj force-pushed the split-configured-clients-strategy branch from 7050efb to c46491d Compare October 14, 2020 11:15
@wouterj
Copy link
Contributor Author

wouterj commented Oct 14, 2020

CHANGELOG entry added.

@dbu dbu merged commit bcd1e38 into php-http:master Oct 21, 2020
@dbu
Copy link
Collaborator

dbu commented Oct 21, 2020

thanks a lot!

@dbu
Copy link
Collaborator

dbu commented Oct 21, 2020

thanks, tagged 1.19.0

@wouterj wouterj deleted the split-configured-clients-strategy branch October 21, 2020 08:26
@wouterj
Copy link
Contributor Author

wouterj commented Oct 21, 2020

Thanks for the merge & quick release!

@blackarcanis
Copy link

Hi ! Just got this error message when moving to 1.19.0 today:
Failed to create closure from callable: class 'Http\HttplugBundle\Discovery\ConfiguredClientsStrategy' does not have a method 'onEvent'

Any idea ?
Thank you !

@dbu
Copy link
Collaborator

dbu commented Oct 23, 2020

hi @blackarcanis , do you redefine the services of this bundle somehow? are you sure that the container has been rebuilt for the env in which you encounter the error? if this is with env=prod, can you try bin/console --env=prod cache:clear ?

@wouterj wouterj mentioned this pull request Oct 24, 2020
Closed
@blackarcanis
Copy link

blackarcanis commented Oct 30, 2020 •
edited
Loading

@dbu Hum yes and only got this issue in our production. Seems to be due to our version of SentryBundle v2.3.0...
I'm looking at this.

@Ziyann
Copy link

Ziyann commented Oct 31, 2020

@blackarcanis, I had to manually remove the old cache on production to fix that issue:
rm -rf var/cache/prod && bin/console cache:clear

@dbu
Copy link
Collaborator

dbu commented Oct 31, 2020

do you deploy to a new directory and build the cache, or simply overwrite the live code and then run bin/console cache:clear on an already warmed up cache? if the later, then i think Ziyann's suggestion is a good solution. (and if you are concious about uptime of your site, i'd recommend looking into deployment strategies with symlinks and stuff, but that is kinda off topic ;-) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wouterj @dbu @blackarcanis @Ziyann