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.

Sign up for GitHub

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

Jump to bottom

support different property paths for read and write #1

Merged
merged 1 commit into from
Jul 11, 2018
Merged

support different property paths for read and write #1

merged 1 commit into from
Jul 11, 2018

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jun 15, 2018 •
edited

This will resolve #3.

@xabbuh xabbuh requested a review from chr-hertel June 15, 2018 18:21
@xabbuh xabbuh force-pushed the read-write-property-path-data-mapper branch from 9bbaa2a to 8c54ab0 Compare June 17, 2018 08:57
chr-hertel
chr-hertel reviewed Jun 17, 2018
View reviewed changes
src/DataMapper/PropertyPathDataMapper.php Outdated
private $dataMapper;
private $propertyAccessor;

public function __construct(DataMapperInterface $dataMapper, ?PropertyAccessorInterface $propertyAccessor = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is nullable type-hint and null as default needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot the handling of null to have this part inline with Symfony's built-in PropertyPathMapper:

$this->propertyAccessor = $propertyAccessor ?? PropertyAccess::createPropertyAccessor();

This is fixed now.

chr-hertel
chr-hertel reviewed Jun 17, 2018
View reviewed changes
src/DataMapper/PropertyPathDataMapper.php Outdated

foreach ($forms as $form) {
if (null !== $readPropertyPath = $form->getConfig()->getOption('read_property_path')) {
$data = $this->propertyAccessor->getValue($data, $readPropertyPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

so this could be getValue on null?

Copy link
Member Author

Choose a reason for hiding this comment

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

see above

chr-hertel
chr-hertel reviewed Jun 17, 2018
View reviewed changes
src/Extension/RichModelFormsTypeExtension.php Outdated

public function buildForm(FormBuilderInterface $builder, array $options): void
{
if (null !== $dataMapper = $builder->getDataMapper()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

early exit instead? we already know this is going to be more in here

Copy link
Member Author

Choose a reason for hiding this comment

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

check

@xabbuh xabbuh force-pushed the read-write-property-path-data-mapper branch 4 times, most recently from ef417ce to d7ac03c Compare June 24, 2018 10:05
dbrumann
dbrumann approved these changes Jun 26, 2018
View reviewed changes
src/DataMapper/PropertyPathDataMapper.php Outdated
$empty = null === $data || [] === $data;

if (!$empty && !is_array($data) && !is_object($data)) {
throw new UnexpectedTypeException($data, 'object, array or empty');
Copy link

Choose a reason for hiding this comment

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

Maybe use null instead of empty as someone might assume it also allows an empty string.

tests/DataMapper/PropertyPathDataMapperTest.php
}
}

class Foo
Copy link

Choose a reason for hiding this comment

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

Maybe move this dummy data to a separate file?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can think about that when we need to reuse the class in other tests IMO. Right now I don't want to expose this dummy data.

Copy link
Member Author

Choose a reason for hiding this comment

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

see #7

spec/Extension/RichModelFormsTypeExtensionSpec.php Outdated
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;

class RichModelFormsTypeExtensionSpec extends ObjectBehavior
Copy link

Choose a reason for hiding this comment

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

Is there any specific reason for having both phpspec and phpunit? I think mixing them might raise the barrier for future contributors.

Copy link
Member Author

Choose a reason for hiding this comment

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

see #2

@xabbuh xabbuh force-pushed the read-write-property-path-data-mapper branch 2 times, most recently from a38e9bb to 6f1c8dc Compare June 26, 2018 18:19
@xabbuh xabbuh changed the title [WIP] support for differing read and write property paths support different property paths for read and write Jun 26, 2018
@chr-hertel
Copy link
Contributor

right now the extension expects that read and write path are used together, but do we really need to enforce that?

chr-hertel
chr-hertel approved these changes Jun 26, 2018
View reviewed changes
Copy link
Contributor

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

possible follow ups:

  • change name of data mapper
  • decouple read and write path

@xabbuh
Copy link
Member Author

xabbuh commented Jun 27, 2018

right now the extension expects that read and write path are used together, but do we really need to enforce that?

Right now I would stick with it. We can reconsider this decision when we have a clear use case where only a read or write path seems reasonable.

We could even go one step further and throw an exception when the default property_path option is used with any of the newly added ones.

@xabbuh xabbuh force-pushed the read-write-property-path-data-mapper branch from 6f1c8dc to cf0198d Compare July 11, 2018 15:19
@xabbuh xabbuh merged commit cf0198d into master Jul 11, 2018
xabbuh added a commit that referenced this pull request Jul 11, 2018
@xabbuh
feature #1 support different property paths for read and write (xabbuh)
2d53fc6 
This PR was merged into the 0.1-dev branch.

Discussion
----------

support different property paths for read and write

This will resolve #3.

Commits
-------

cf0198d support different property paths for read and write
@xabbuh xabbuh deleted the read-write-property-path-data-mapper branch July 11, 2018 15:21
@xabbuh xabbuh added this to the 1.0 milestone Jul 13, 2018
This was referenced Jul 13, 2018
Closed
Closed
@xabbuh
Copy link
Member Author

xabbuh commented Jul 13, 2018

see #8 and #9 for the follow-up issues

@xabbuh xabbuh added the feature label Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chr-hertel chr-hertel chr-hertel approved these changes

@dbrumann dbrumann dbrumann approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

support different property paths for read and write operations
3 participants
@xabbuh @chr-hertel @dbrumann