This repository has been archived by the owner on Jan 30, 2020. It is now read-only.
Merge ArrayMapNamingStrategy into MapNamingStrategy #82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
froschdesign
approved these changes
Nov 29, 2018
Ocramius
suggested changes
Nov 30, 2018
weierophinney
force-pushed
the
feature/9-merge-map-naming-strategies
branch
from
c179169
to
8464c4c
Compare
|
@Ocramius I've created three named constructors:
This solved the various issues I had with phpstan, and, as you note, it makes the behavior far more clear. I've added tests to check for non-string keys as well; these are almost like those for values, but I had to remove the Thanks for the review! |
weierophinney
added a commit
to weierophinney/zend-hydrator
that referenced
this pull request
Dec 3, 2018
Per zendframework#9, the two perform almost identical functionality; the main difference is _which argument is first in the constructor_, and the fact that `MapNamingStrategy` allowed a second argument for the opposite direction. This patch merges the two into `MapNamingStrategy`. The class is now final (as was `ArrayMapNamingStrategy`, and accepts either a `$hydrationMap` or an `$extractionMap` or both. If both are missing, it raises an exception. If one or the other is missing, the value becomes that of the `array_flip` of the other value present. The patch includes a migration document section detailing the changes, and expands the `MapNamingStrategy` documentation to cover all use cases.
…ctor phpstan was complaining that the value passed to `flipMapping()` could be null, and that the assignment to each of `extractionMap` and `hydrationMap` could potentially be null but should not be. This patch adds explicit `is_array()` checks before attempting to call `flipMapping()` with a value, and casting to `array` during assignment to properties.
Removes the existing constructor, and creates three named constructors: - `public static function createFromExtractionMap(array $extractionMap) : self` - `public static function createFromHydrationMap(array $hydrationMap) : self` - `public static function createFromAssymetricMap(array $extractionMap, array $hydrationMap) : self` Doing so removes all the various constructor checks we were previously using. The patch also updates `flipMapping()` as follows: - It requires string values _only_; integer are not allowed. - It also checks the keys, as they can only be string values. Finally, it updates the typehints on both the extraction and hydration maps to `array<string, string`, as the keys are also important.
weierophinney
force-pushed
the
feature/9-merge-map-naming-strategies
branch
from
966fe17
to
7cabf9c
Compare
Ocramius
reviewed
Dec 3, 2018
There was a problem hiding this comment.
@weierophinney I don't see a private function __construct for the MapNamingStrategy
|
Moved discussion point into #85 |
|
Feedback handled in #86 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per #9, the two perform almost identical functionality; the main difference is which argument is first in the constructor, and the fact that
MapNamingStrategyallowed a second argument for the opposite direction.This patch merges the two into
MapNamingStrategy. The class is now final (as wasArrayMapNamingStrategy, and accepts either a$hydrationMapor an$extractionMapor both. If both are missing, it raises an exception. If one or the other is missing, the value becomes that of thearray_flipof the other value present.The patch includes a migration document section detailing the changes, and expands the
MapNamingStrategydocumentation to cover all use cases.Fixes #9