Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should statement/execute return result instead of statement object? #3

Closed
jasalt opened this issue Dec 17, 2024 · 2 comments · Fixed by #5
Closed

Should statement/execute return result instead of statement object? #3

jasalt opened this issue Dec 17, 2024 · 2 comments · Fixed by #5

Comments

@jasalt
Copy link
Contributor

jasalt commented Dec 17, 2024

I noted that when inserting something with statement/execute, I would expect to receive the insertion result (e.g. true) coming from (php/-> (stmt :stmt) (execute)) which is now discarded with the original statement being returned in

statement)
.

This leaves no other way to know if insertion was successful than looking for a PDOException if I got it correct. Maybe this could be considered while improving this library.

Example situation where I noticed this being an issue while making function allowing to insert map into table:

  (defn pdo-insert-map  # work in progress function
    [conn table m]
    (let [m-keys (keys m)
          cols (map name m-keys)
          params (str/join ", " m-keys)
          stmt-sql (str "INSERT INTO " table
                        " (" (str/join ", " cols) ") VALUES ("
                        params ")")
          stmt (pdo/prepare pdo-conn stmt-sql)]

      (dofor [[k v] :pairs m]
             (statement/bind-value stmt k v))

      (statement/execute stmt)
      # (php/-> (stmt :stmt) (execute))  # with this instead, true is returned
      ))

  (pdo-insert-map pdo-conn "my_table"
                {:uuid        "testuuid"
                 :create_date "2024-01-01 11:11:11"
                 :order_id    "234"
                 :qty_sold    "5"
                 })
  
  # -> (phel\pdo\statement\statement Printer cannot print this type: PDOStatement)
@jasalt jasalt changed the title Should statement/execute return result from (php/-> (stmt :stmt) (execute))? Should statement/execute return result instead of statement object? Dec 17, 2024
@jasalt
Copy link
Contributor Author

jasalt commented Dec 18, 2024

Needs some thinking as statement->rowCount() would make more sense in the example code sense (#4 (comment))..

Maybe it would be better to keep current statement/execute behavior as is and to just figure out / document more usage patterns around it. I'm more familiar to JDBC use with Clojure but new to PDO so still discovering how it works in general.

@smeghead
Copy link
Member

Thank you very much.

Indeed, as a simple PDO wrapper library, it might be more appropriate for statement/execute to return a bool value.

However, the pho/connect function explicitly specifies PDO::ERRMODE_EXCEPTION, so an exception should be thrown in case of failure.

(set-attribute conn (php/:: \PDO ATTR_ERRMODE) (php/:: \PDO ERRMODE_EXCEPTION))

Therefore, we think it is impossible for the return value of statement/execute to be false.

Given these considerations, I would like to document that the return value of statement/execute is not a bool value without changing the current behavior.

smeghead added a commit that referenced this issue Dec 29, 2024
doc: add note about return value of `statement/execute` #3
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 a pull request may close this issue.

2 participants