Add Symfony 3.4 compatibility, without deprecation notices #47
Conversation
ChristianRiesen
changed the title
| */ | ||
| public function reset() | ||
| { | ||
| $this->data = []; |
There was a problem hiding this comment.
I'd rather do:
$this->data['commands'] = [];(because this is what is done in __construct())
but maybe I misunderstood the "reset" purpose?
There was a problem hiding this comment.
It should be the entire data, not just commands.
There was a problem hiding this comment.
this DataCollector seems to require the commands key for data array
otherwise we'll have some issues with other methods (i.e. getCommands, getTotalExecutionTime and getAvgExecutionTime)
There was a problem hiding this comment.
I'll have a closer look at it again today to make sure this works as intended.
|
@ChristianRiesen could you please also do what has been done in https://github.com/M6Web/AmqpBundle/pull/46/files please? 🙏 If this bundle could be compatible with SF4 too, that would be nice :) Or we could merge your PR before merging #46, as you want |
|
@Oliboy50 There is a problem with Symfony 4, that's why I backed it down to 3.4 to be compatible, so that can be used without having tons of deprecation notices. For 4.0 you need a bit more work. I had it in originally, but that went bad. See here: https://travis-ci.org/M6Web/AmqpBundle/builds/312455080 I'd be very happy to have this merged as is and tagged. My suspicion is that there is something going wrong with the tests because of how PHP 7.1 handles the error triggering now, but that's just a hunch, and I think it would be best to address that in another PR. |
|
@ChristianRiesen thanks for your feedback about SF4 👍 |
|
No problem. It gave me some headaches today, but thankfully I don't need SF4, yet :) |
|
@Oliboy50 I went over it again and made sure there will be no states that could cause an error. I cleaned up some small things (also in regard to PHP7) in this PR as well since probably nobody will touch the datacollector in a long time anyways :) If this works for you, please merge and tag. Thank you! |
| { | ||
| return ($this->getTotalExecutionTime()) ? ($this->getTotalExecutionTime() / count($this->data['commands'])) : 0; | ||
| $this->data = []; |
There was a problem hiding this comment.
could be $this->data = ['commands' => []]; and remove the next line
There was a problem hiding this comment.
True, let me fix that quickly.
There was a problem hiding this comment.
Changed the line. Thanks for catching that :)
Added small cleanups and PHP 7 enhancements while I was at it to the datacollector.
| { | ||
| return ($this->getTotalExecutionTime()) ? ($this->getTotalExecutionTime() / count($this->data['commands'])) : 0; | ||
| $this->data = [['commands' => []]; |
There was a problem hiding this comment.
Don't you need to call parent::reset() to ensure proper $this-data reset? 🤔
parent::reset();
$this->data['commands'] = [];I didn't find where is defined data and who is in charge of its default initialization 😕
There was a problem hiding this comment.
Not really, most other data collectors I ran into so far do just reset to empty array, as do most of the official ones from symfony itself.
There was a problem hiding this comment.
Sounds good 😃
Let's go for v3.0.1 🎉
Added reset function that caused deprecation notices in Symfony 3.4+ upgrades.