Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented May 10, 2018

I noticed when I wanted to log the queries we make that not all of them are logged. Then I remembered that the Mapper doesn't use the query builder yet. So I decided to add a new one.

  • Add new QBMapper
  • Deprecate old Mapper
  • Moved over DefaultTokenMapper

We should probably add tests.

rullzer added 3 commits May 10, 2018 19:47
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the techdep/noid/appframework_mapper_to_qb branch from 1a4261e to 610c665 Compare May 10, 2018 17:47
@codecov
Copy link

codecov bot commented May 10, 2018

Codecov Report

Merging #9444 into master will decrease coverage by 0.03%.
The diff coverage is 6.66%.

@@             Coverage Diff              @@
##             master    #9444      +/-   ##
============================================
- Coverage     51.61%   51.57%   -0.04%     
- Complexity    25694    25712      +18     
============================================
  Files          1638     1639       +1     
  Lines         96318    96398      +80     
  Branches       1393     1393              
============================================
+ Hits          49712    49716       +4     
- Misses        46606    46682      +76
Impacted Files Coverage Δ Complexity Δ
...rivate/Authentication/Token/DefaultTokenMapper.php 100% <ø> (ø) 11 <0> (ø) ⬇️
lib/public/AppFramework/Db/Mapper.php 94.95% <ø> (-4.17%) 30 <0> (ø)
lib/public/AppFramework/Db/QBMapper.php 6.66% <6.66%> (ø) 18 <18> (?)
apps/files_trashbin/lib/Expiration.php 90.32% <0%> (-1.62%) 29% <0%> (ø)

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 great

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 👍 - please add it to the docs.

$getter = 'get' . ucfirst($property);
$value = $entity->$getter();

$qb->setValue($column, $qb->createNamedParameter($value));
Copy link
Member

@MorrisJobke MorrisJobke May 14, 2018

Choose a reason for hiding this comment

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

SqlInjectionChecker Potential SQL injection detected - method: no child method

$getter = 'get' . ucfirst($property);
$value = $entity->$getter();

$qb->set($column, $qb->createNamedParameter($value));
Copy link
Member

Choose a reason for hiding this comment

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

SqlInjectionChecker Potential SQL injection detected - method: no child method

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 14, 2018
@rullzer rullzer merged commit 497a4fa into master May 14, 2018
@rullzer rullzer deleted the techdep/noid/appframework_mapper_to_qb branch May 14, 2018 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants