Skip to content

Add an option for skip type conversion#1234

Closed
cve wants to merge 3 commits into
api-platform:masterfrom
cve:skip_convert_identifier
Closed

Add an option for skip type conversion#1234
cve wants to merge 3 commits into
api-platform:masterfrom
cve:skip_convert_identifier

Conversation

@cve

@cve cve commented Jul 6, 2017

Copy link
Copy Markdown
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1167, #1233
License MIT
Doc PR None

I'm added an option to ApiProperty for attributes with "convert"=false, which skip converting doctrine type

@dunglas @sroze @theofidry

@cve cve changed the title Add and option for skip type conversion Add an option for skip type conversion Jul 6, 2017

@dunglas dunglas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you also add at least a unit test?

*/
private function shouldConvert(array $attributes = null): bool
{
foreach ((array) $attributes as $attribute) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use less cast because of the typehint.

@cve cve Jul 10, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

it seems to me, If attributes will be null then invalid argument supplied to foreach will be thrown, but if I do array attributes = [] then the cast will be use less, you are right, I will fix it, thanks

}

$doctrineTypeName = $doctrineClassMetadata->getTypeOfField($propertyName);
if ($this->shouldConvert($propertyMetadata->getAttributes())) {

@dunglas dunglas Jul 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't you use the getAttribute method directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cause I didn't know about that :), I will fix it, thanks.

@Simperfit

Copy link
Copy Markdown
Contributor

Could you finish this please ?

@Simperfit Simperfit closed this May 2, 2018
@Simperfit Simperfit reopened this May 2, 2018
@cve

cve commented May 16, 2018

Copy link
Copy Markdown
Author

#1478 is not it better?

@soyuka

soyuka commented May 16, 2018

Copy link
Copy Markdown
Member

👍 #1478 should cover that use case, thanks @cve !

@cr3a7ure cr3a7ure mentioned this pull request Oct 21, 2018
This was referenced Nov 5, 2018
@Mathieudewet Mathieudewet mentioned this pull request Jul 8, 2019
4 tasks
@aaquib19 aaquib19 mentioned this pull request Sep 30, 2019
@chalasr chalasr mentioned this pull request Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants