Fix info headers Extend error in dependant libs #304
Conversation
| @@ -54,7 +54,9 @@ | |||
| <exclude name="Generic.Functions.OpeningFunctionBraceKernighanRitchie"/> | |||
| </rule> | |||
| <rule ref="Generic.Metrics"/> | |||
| <rule ref="Generic.NamingConventions"/> | |||
| <rule ref="Generic.NamingConventions"> | |||
There was a problem hiding this comment.
PHP formatting directive
| * | ||
| * @var array | ||
| */ | ||
| protected $data = []; | ||
| protected $data = [ 'name' => 'Not set (PHP)', 'version' => 'Not set (PHP)' ]; |
There was a problem hiding this comment.
Default package data to call out if setPackage is not being called. Worth it to look into setting these in a constructor at some point in the future.
There was a problem hiding this comment.
I rather have whoever reads and pulls this data to add defaults in a transformation rather than sending "defaults" when missing or not set on every request.
There was a problem hiding this comment.
@lbalmaceda - Can't do a transformation without a name present. I think this will help to figure out if we have untracked entries and where they came from.
There was a problem hiding this comment.
On second thought ... I think you're right. This is jumping the gun on addressing untracked SDKs. I'll pull this out tomorrow, just me know if you have any other feedback.
There was a problem hiding this comment.
In PHP, why wouldn't you be able to ask for the PHP version?
http://php.net/manual/en/function.phpversion.php
There was a problem hiding this comment.
@cocojoe - This is the SDK version and has to be set explicitly.
| 'name' => $name, | ||
| 'version' => $version, | ||
| ]; | ||
| $this->setEnvProperty($name, $version); |
There was a problem hiding this comment.
Make sure we're using the correct env format here. Deprecated but needs to be refactored in other libs before it can be removed.
| 'name' => $name, | ||
| 'version' => $version, | ||
| ]; | ||
| $this->setEnvProperty($name, $version); |
There was a problem hiding this comment.
Deprecated but make sure we're capturing the data here if it's being used anywhere else.
|
|
||
| $oldData = $headers->get(); | ||
| if (! empty( $old_headers['env'] ) && is_array( $old_headers['env'] )) { |
There was a problem hiding this comment.
Check for the existence of the env key before trying to set it.
| * | ||
| * @return void | ||
| */ | ||
| public function testThatSetEnvironmentSetsDataCorrectly() |
There was a problem hiding this comment.
Test for InformationHeaders:: setEnvironment() while it's still being used.
| * | ||
| * @return void | ||
| */ | ||
| public function testThatExtendedHeadersBuildCorrectly() |
There was a problem hiding this comment.
Test for dependent lib behavior (AKA: testing what broke)
| * | ||
| * @var array | ||
| */ | ||
| protected $data = []; | ||
| protected $data = [ 'name' => 'Not set (PHP)', 'version' => 'Not set (PHP)' ]; |
There was a problem hiding this comment.
I rather have whoever reads and pulls this data to add defaults in a transformation rather than sending "defaults" when missing or not set on every request.
| class InformationHeaders | ||
| { | ||
|
|
||
| /** | ||
| * Default header data to send. | ||
| * | ||
| * @var array | ||
| */ | ||
| protected $data = []; |
There was a problem hiding this comment.
@lbalmaceda - Fixed up!
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
envdata before extending.Reported in auth0/laravel-auth0#108
Closes #303