Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Broader PDO fetch-style range #296

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Broader PDO fetch-style range #296

wants to merge 10 commits into from

Conversation

chris-kruining
Copy link

I encountered a situation Where I want to fetch multiple columns with the same name where aliases are not an option, so I set the fetchmode to \PDO::FETCH_NAMED which resulted in the Exception\InvalidArgumentException "The fetch mode must be one of the PDO::FETCH_* constants.", which \PDO::FETCH_NAMED is.

I've attempted to fix this by editing the condition so that all the fetch styles listed in http://php.net/manual/en/pdostatement.fetch.php

I hope that this PR is to the required standards :D

@ezimuel
Copy link
Contributor

ezimuel commented Aug 7, 2018

@chris-kruining thanks for this PR, I'm sorry to review it only now. The goal of your PR is fine (@Ocramius as library, we need to support all the PDO fetch modes), but I think it's better to check for valid PDO::FETCH_* constants using an array of allowed modes.
Something like this:

use PDO;

class Result implements Iterator, ResultInterface
{
  // ...
  private $allowedFetchModes = [
    PDO::FETCH_ASSOC,
    PDO::FETCH_BOTH,
    PDO::FETCH_BOUND,
    PDO::FETCH_CLASS,
    PDO::FETCH_CLASSTYPE,
    PDO::FETCH_PROPS_LATE,
    PDO::FETCH_INTO,
    PDO::FETCH_LAZY,
    PDO::FETCH_NAMED,
    PDO::FETCH_NUM,
    PDO::FETCH_OBJ
  ];

  /**
   * @param int $fetchMode
   * @throws Exception\InvalidArgumentException on invalid fetch mode
   */
  public function setFetchMode($fetchMode)
  {
      if (! in_array($fetchMode, $this->allowedFetchModes)) {
          throw new Exception\InvalidArgumentException(
              'The fetch mode must be one of the PDO::FETCH_* constants.'
           );
      }
      $this->fetchMode = (int) $fetchMode;
  }
}

This change is required also for fix an issue on the previous implementation. In fact, checking for $fetchMode < 1 || $fetchMode > 10 is not enough. For instance 7 is not a valid PDO::FETCH_* mode.

@chris-kruining can you change this PR according to my suggestion? Thanks a ton!

@chris-kruining
Copy link
Author

@ezimuel I've made the changed like you proposed, however I do have some concerns:

In my opinion is not a completely valid set as \PDO::FETCH_PROPS_LATE and \PDO::FETCH_CLASSTYPE should only be used in combination with \PDO::FETCH_CLASS

@chris-kruining
Copy link
Author

@ezimuel ping

@chris-kruining
Copy link
Author

Would be great if this could be resolved guys, I think 1.5 years is a tad bit long :P

CHANGELOG.md Outdated Show resolved Hide resolved
src/Adapter/Driver/Pdo/Result.php Outdated Show resolved Hide resolved
@chris-kruining
Copy link
Author

@webimpress the issues you have mentioned should have been resolves afaik

@michalbundyra
Copy link
Member

@chris-kruining Thanks, but now somehow I see conflicts on this PR. Can you resolve them, please?

@chris-kruining
Copy link
Author

@webimpress fixed

@chris-kruining
Copy link
Author

ping

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#59.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants