Closed Chrico closed 4 years ago
The problem with exposing inner objects is that (besides breaking encapsulation) consumers can access ProviderStatus
objects from the debug info and act on them, breaking things in non-predictable ways: debug info method becomes at any effect a getter of encapsulated objects.
This was also the reason why there was a cast to string: anyone obtaining a string can't do anything on that besides printing, obtaing objects consumers can call methods on those object breaking expectations.
The same apply for exposing Context
.
Regarding SiteConfig
it will likely contain secrets, and adding secrets to method designed for debug does not look a good idea to me.
If we need more debug info, we could do that without breaking encapsulation and risking of exposing secrets:
ProviderStatus
could be made JsonSerializable
with a jsonSerialize
that returns $this->appStatuses
: that is a <string, string>
map that is safe to output.AppLogger
instead of return array_map('strval', $this->providers);
could then do return array_map('jsonSerialize', $this->providers);
App::debugInfo()
instead of returning $this->container->context()
could return $this->container->context()->jsonSerialize()
(because Context
is already JsonSerializable
). After that, SiteConfig
could then extend JsonSerializable
and in EnvConfig::jsonSerialize()
we could return ['env' => $this->env, 'keys' => array_keys($this->data)];
so that we would expose the environment name and the keys of retrieved config, both safe to output. At that point, App::debugInfo()
could also return $this->container->config()->jsonSerialize()
.@Chrico I implemented the things I described in febd9ec (I also needed to fix tests). Please review.
Then I also pushed 88522b0 to upgrade and fix PHPCS and fix Psalm. I also added testing.yml
to enable GitHub actions.
Thanks! Originally i wanted start working on that today..but seem's i now only need to review ;)
I've enhanced the
App::debugInfo
to provide more information about theContainer::context
andContainer::config
.Also i've removed the string casting on
AppLogger::dump
onProviderStatus
. This will still work when just printing theProviderStatus
due the__toString()
-method. But also now allows to access more in detail viaProviderStatus::status
andProviderStatus::appStatuses
the actual stored values.The pull contains following:
AppLogger // dump method does not cast string on ProviderStatus. ProviderStatus // add new methods status and appStatuses to expose more information to outside. App::debugInfo new contains context and config from container.