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.

None yet

5 participants