Skip to content

Conversation

@marius-wieschollek
Copy link
Contributor

@marius-wieschollek marius-wieschollek commented Mar 24, 2019

This patch sets the parameter type in the insert and update methods of QBMapper based on the field types declared in the Entity. Parameters that can for any reason not be mapped will be mapped to "string" which was also the default before.

Fixes #11236

@MorrisJobke
Copy link
Member

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code makes sense and looks good 👍

@MorrisJobke MorrisJobke changed the title [#11236] Set parameter type in QBMapper Set parameter type in QBMapper Mar 25, 2019
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good and has tests 🚀 👍

* @param Entity $entity The entity to get the types from
* @param string $property The property of $entity to get the type for
* @return int
* @since 16.0.0
Copy link
Member

Choose a reason for hiding this comment

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

protected method, no @since necessary

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me - it's just additional documentation that's fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i do not add a @since tag, autotest will not accept it

$ ./autotest.sh sqlite lib/AppFramework/Db/QBMapperTest.php
Using PHP executable /usr/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

@since or @deprecated tag is needed in PHPDoc for OCP\AppFramework\Db\QBMapper::getParameterTypeForProperty

Copy link
Member

Choose a reason for hiding this comment

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

okidoke, no problem :)

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 25, 2019
@MorrisJobke MorrisJobke merged commit c57a16b into nextcloud:master Mar 25, 2019
@welcome
Copy link

welcome bot commented Mar 25, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

@rullzer rullzer mentioned this pull request Mar 26, 2019
9 tasks
@marius-wieschollek marius-wieschollek deleted the bugfix/11236 branch January 4, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug feature: install and update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants