Refactor exception handling of Binary\Loader\ChainLoader class
#1434
Conversation
robfrawley
added
Level: Enhancement ✨
| private string $configName; | ||
| private LoaderInterface $objectInst; | ||
|
|
||
| public function __construct(string $configName, LoaderInterface $objectInst, NotLoadableException $loaderException) |
There was a problem hiding this comment.
| public function __construct(string $configName, LoaderInterface $objectInst, NotLoadableException $loaderException) | |
| public function __construct(string $configName, LoaderInterface $loader, NotLoadableException $loaderException) |
same here
There was a problem hiding this comment.
@franmomu @dbu Are you both comfortable with changing objectInst to loaderInst (instead of the suggested variable name of loader) with regard to the exception class only? Specifically:
ChainAttemptNotLoadableException::$objectInst-> the original property nameChainAttemptNotLoadableException::$loaderInst-> my suggested property name
This better aligns with the methods as they are currently defined for the chain-attempt exception, IMHO. For example:
ChainAttemptNotLoadableException::getLoaderConfigName()ChainAttemptNotLoadableException::getLoaderObjectInst()ChainAttemptNotLoadableException::getLoaderObjectName()
(Not that it would matter to anyone else, but my suggestion also better satisfies my OCD which prefers same-length groups of properties and methods, when feasible to do so without convoluting their meaning...)
Let me know if this is objectionable to either of you; I'll perform the original change if you both are not agreeable to my suggested alternative.
src/Exception/Binary/Loader/ChainAttemptNotLoadableException.php
Outdated
Show resolved
Hide resolved
|
i like this refactoring, thanks! i agree with the comment from @franmomu about naming the loader variable $loader and making things final where possible. |
…le\Binary\Loader\ChainLoader: - refactored Liip\ImagineBundle\Binary\Loader\ChainLoader error handling logic to exception classes instead of prior half-baked and confusing handling inside loader inself - testing alternate ChainNotLoadableException::compileExceptionMessage() implementation - cleanup of refactored Liip\ImagineBundle\Binary\Loader\ChainLoader and its related exceptions - ran php-cs-fixer on prior ChainLoader related changes - updates per reviews from @dbu and @franmomu
robfrawley
force-pushed
the
feature-refactor-chain-loader-error-handling
branch
from
ac371a7
to
e49602b
Compare
robfrawley
changed the title
Binary\Loader\ChainLoader classBinary\Loader\ChainLoader class
|
I've gone ahead and squashed the five commits created while working toward this PR, as well as rebased my changes over the latest (At this point, I'll squash and rebase every time any additional changes are required so the PR is always in a clean and easily mergeable state; hopefully such doesn't happen too many times, though :-)) |
| return $this->loaderInst; | ||
| } | ||
|
|
||
| public function getLoaderObjectName(): string |
There was a problem hiding this comment.
if i understand correctly, this is the class name without the namespace. how about calling the method getLoaderClassName?
| return $this->configName; | ||
| } | ||
|
|
||
| public function getLoaderObjectInst(): LoaderInterface |
There was a problem hiding this comment.
yeah naming things... i would call the thing just $loader and this method getLoader. i prefer concise names. that it is an instantiated object is defined by the return type declaration.
| final class ChainAttemptNotLoadableException extends NotLoadableException | ||
| { | ||
| private string $loaderIndex; | ||
| private LoaderInterface $loaderClass; |
There was a problem hiding this comment.
i would prefer to call this $loader. $loaderClass suggests its the class name of the loader, but it is the actual instance.
then again, looking at the rest of the code: we do not use the class anywhere afterwards. how about doing the reflection thing in the constructor and have this field be private string $loaderClassName?
| return $this->loaderIndex; | ||
| } | ||
|
|
||
| public function getLoaderClass(): LoaderInterface |
There was a problem hiding this comment.
this is not called from outside the class, i think we can drop the method
| return $this->loaderClass; | ||
| } | ||
|
|
||
| public function getLoaderClassName(): string |
There was a problem hiding this comment.
but this one should stay as is, it gives you the classname of the loader, so i like the name.
if we move reflection to the constructor, this will become a simple getter for $this->loaderClassName
This is a work in progress toward refactoring the error handling logic of
Liip\ImagineBundle\Binary\Loader\ChainLoaderas was originally discussed in code review comments from #1432; see the discussion beginning with dbu's #1432 comment and, more specifically, my (robfrawley) #1432 comment in response.Additional information will be added as my schedule permits and during this PR's progression.
For the time being, I would greatly appreciate any thoughts from @dbu, @franmomu, or any other maintainer/contributor/user as to the current state of this pull request as well as any alternate possible directions to investigate.