Skip to content
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.

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

Refactor exception handling of Binary\Loader\ChainLoader class #1434

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

robfrawley
Copy link
Collaborator

Q A
Branch? 3.x
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
References #1432 (comment) / #1432 (comment)
License MIT
Doc n/a

This is a work in progress toward refactoring the error handling logic of Liip\ImagineBundle\Binary\Loader\ChainLoader as 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.

@robfrawley robfrawley added Level: Enhancement ✨ This item involves an enhancement to existing functionality. Type: Source Code This item pertains to the source code of this project. labels Nov 23, 2021
@robfrawley robfrawley self-assigned this Nov 23, 2021
Copy link
Contributor

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

Nice refactor!

src/Binary/Loader/ChainLoader.php Outdated Show resolved Hide resolved
private string $configName;
private LoaderInterface $objectInst;

public function __construct(string $configName, LoaderInterface $objectInst, NotLoadableException $loaderException)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function __construct(string $configName, LoaderInterface $objectInst, NotLoadableException $loaderException)
public function __construct(string $configName, LoaderInterface $loader, NotLoadableException $loaderException)

same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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 name
  • ChainAttemptNotLoadableException::$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.

@dbu
Copy link
Member

dbu commented Nov 25, 2021

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 robfrawley force-pushed the feature-refactor-chain-loader-error-handling branch from ac371a7 to e49602b Compare November 25, 2021 18:37
@robfrawley robfrawley changed the title [WIP] Refactor exception handling of Binary\Loader\ChainLoader class Refactor exception handling of Binary\Loader\ChainLoader class Nov 25, 2021
@robfrawley
Copy link
Collaborator Author

I've gone ahead and squashed the five commits created while working toward this PR, as well as rebased my changes over the latest 3.x branch. I think this should be just about ready, pending the outcome of #1434 (comment) and any additional thoughts that anyone may have after performing any final reviews. As such, I've removed the [WIP] label from the PR title and I request a final quick check from any interested maintainers and contributors.

(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 :-))

Copy link
Member

@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 would prefer to do some renaming here as i feel both "object" and "inst" do not help understanding the code, so just "loader" would be my preferred solution.

take it as a suggestion, if you really want the current name, we can also keep it.

return $this->loaderInst;
}

public function getLoaderObjectName(): string
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

…or handling implementation and updates per reviews from @dbu and @franmomu
Copy link
Member

@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.

sorry to continue to argue about the naming 🙈

i think we can further simplify the exception class and remove some methods we don't need.

final class ChainAttemptNotLoadableException extends NotLoadableException
{
private string $loaderIndex;
private LoaderInterface $loaderClass;
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

this is not called from outside the class, i think we can drop the method

return $this->loaderClass;
}

public function getLoaderClassName(): string
Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: Enhancement ✨ This item involves an enhancement to existing functionality. Type: Source Code This item pertains to the source code of this project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants