Skip to content

New Functions for Active Record & Fixes - #37

Open
enlivenapp wants to merge 10 commits into
flightphp:masterfrom
enlivenapp:master
Open

New Functions for Active Record & Fixes#37
enlivenapp wants to merge 10 commits into
flightphp:masterfrom
enlivenapp:master

Conversation

@enlivenapp

Copy link
Copy Markdown
Contributor

I'm back! Been continuing work on my project(Pubvana v3) and using AR for the database interaction. In this PR I've done my best to segregate what Pubvana needs to what should actually be in an AR library. Hopefully I've gotten it right (EG: No query builder UNION type stuff) if you'd like to add this to the repo. -cheers

Bulk addition of new features and some cleanup to the Active Record library. All changes build on security/identifier-escaping work already on master. I did tag as 0.8.0 given the gaggle of changes. Numbers here correspond to my internal branches referenced throughout below.

New Functions:

  1. Aggregate queries
  • count(): int - total rows matching current conditions (ignores grouping, returns a single total)
  • exists(): bool - whether any row matches
  • New private queryScalar() helper
  • DatabaseStatementInterface::fetchColumn() added (breaking for external statement implementors) + PDO/mysqli adapter implementations
  1. Scalar extraction
  • pluck(string $column): array - flat array of values from one column
  • ids(): array - primary keys
  • New private queryColumn() helper
  1. Convenient finders
  • first(): self - first record by PK (ASC)
  • last(): self - last record by PK (DESC)
  • updateAttribute(string $name, $value): self - update a single column on a loaded record
  1. Distinct
  • distinct() chainable modifier (SELECT DISTINCT), composes with pluck()
  1. Batch operations
  • updateAll($attributes): int - bulk update, returns affected row count
  • deleteAll(): int - bulk delete, returns affected row count; no callbacks
  • DatabaseStatementInterface::rowCount() added (breaking for external implementors) + PDO/mysqli adapter implementations
  1. Automatic timestamps
  • protected bool $timestamps - opt-in (false by default); auto-sets created_at/updated_at on insert/update, honoring explicitly set values
  1. Scopes
  • Convention-based named scope instance methods plus optional scope(string $name, ...$args) helper
  1. Transactions
  • transaction(callable $callback) - commit on success, rollback on exception
  • DatabaseInterface::beginTransaction()/commit()/rollback() added (breaking for external implementors) + PDO/mysqli adapter implementations

Found Existing

  1. (9.) Coding-standards cleanup
  • Strict === comparisons in SQL builder;
  • removed leftover debug echo

Docs

  • README updated with examples for all new features
  • Fixed stale documentation URLs to the v3 docs path
  • Breaking changes (for external implementors): see below
  • DatabaseStatementInterface: added fetchColumn() and rowCount()
  • DatabaseInterface: added beginTransaction(), commit(), rollback()

Testing

  • 222 tests, 446 assertions, all passing
  • 100% line coverage on src/
  • PSR-12 (composer phpcs) clean

Breaking changes

Two public interfaces are extended. Because these are interfaces any external user can implement themselves, adding a method to an interface means a custom implementation that doesn't add the method will no longer satisfy the interface, hence "breaking."

DatabaseStatementInterface

Added fetchColumn() (branch 1): previously the statement interface could only execute() and fetch(&$object), both read into an object. There was no driver-agnostic way to read a single scalar/column value, which count(), exists(), pluck(), and ids() all need. Adding:

public function fetchColumn();

means anyone with a custom DatabaseStatementInterface implementation must now implement this method.

Added rowCount() (branch 5): needed to return the number of affected rows from updateAll()/deleteAll(). Same interface-add caveat as above.

DatabaseInterface

Added beginTransaction(), commit(), rollback() (branch 8): required so transaction() can work through the adapter layer regardless of whether the underlying connection is PDO or mysqli. Same interface-add caveat:

public function beginTransaction(): bool;
public function commit(): bool;
public function rollback(): bool;

What this means in practice

  • If you use the built-in PDO/mysqli adapters (the common case), there's nothing to do, the bundled adapters implement all of these.
  • If you have a custom adapter implementing either interface, you'll need to add these methods (and the implementations are one-line calls to the underlying driver).
  • Per the project's versioning, this ships as 0.8.0 to flag the interface additions.

Also, happy to fork the docs and add all this to it as well if you want

- Add fetchColumn() to DatabaseStatementInterface + PDO/mysqli adapters
- Add ActiveRecord::count() and exists() via a private queryScalar() helper
- pluck() returns a single column as a flat array (NULL-safe, order/limit aware)
- ids() delegates to pluck() with the primary key
- queryColumn() reuses the branch-1 fetchColumn() primitive
- isDistinct flag prefixes SELECT DISTINCT on the default table.* select
- pluck() honors the flag; count() deliberately ignores it
- flag resets with the rest of the query state
- rowCount() on DatabaseStatementInterface + PDO/mysqli adapters
- updateAll()/deleteAll() run single statements without hydration or callbacks
- mysqli affected_rows read is @codeCoverageIgnore (same as lastInsertId)
- $timestamps property + protected setTimestamps() hook
- insert() sets both columns, update() sets updated_at only
- explicitly dirty timestamp values are never overwritten
- Named scopes are plain chainable instance methods returning $this
- scope() calls them by name; throws BadMethodCallException when undefined
- beginTransaction()/commit()/rollback() on DatabaseInterface + adapters
- ActiveRecord::transaction() wraps a callable; commit on return, rollback + rethrow on failure
- QueryCountingAdapter fixture updated for the new interface methods
- buildSqlCallback(): strict null comparisons (===)
- buildSql(): remove commented-out debug echo
- rename testScopeHelperThrowsOnUndefined to match plan naming
@ambrose5773

Copy link
Copy Markdown

Hey thanks for the PR! This is a solid set of code changes and tagging 0.8.0 for the interface adds makes sense.

Couple things I have questions in that I noticed.

deleteAll() and updateAll() will wipe the whole table if there’s no WHERE right? They sit right next to delete() / update() on the same object, so $user->deleteAll() is way too easy to mix up with “delete this row.” I’d want those two to refuse to run without a WHERE, or some explicit “yes I mean everything” flag. Would you agree?

updateAll() also takes a raw SET string. A recent change was how there is now identifier escaping, and something like updateAll("password = 'reset'") walks around that. Something like an Array of columns only would keep it consistent.

Thanks again for the assist in maintaining this!

@enlivenapp

Copy link
Copy Markdown
Contributor Author

@ambrose5773

Good catches, both, Here's what I'm considering for fixes:

  1. deleteAll() and updateAll() would wipe the whole table. Given both options you mention, it's trivial to add both. so:
  • with where() : passes
  • deleteAll(true)/updateAll(true): passes
  • with where() and (true): passes
  • neither: fails.

Would both be ok or should one be chosen over the other. It's literally this or a varient:

if ($allowEmptyConditions === false && $this->where === null) {
    throw ...
}

2: I do agree with you, an array is safer.

In looking into it closer I found something a little contradictory to your concern to bring to your attention and ask for some guidance how I might proceed.

where()/having()/select()/order()/group()/from()/join(), ON/set() accept raw fragments (all documented as deliberate escape hatches; orderByColumn() exists as the safe order path) and that's why I chose to go the way I did.

Should these be 'fixed' too while I'm taking care of updateAll()'s SET string? I'm happy to add these in but don't want to get ahead of myself.

Slight side note: I've forked the docs and added these there (and some other things I've written on flight in examples) but I'm holding off on the PR until these were settled so the docs match 100%. Long way to say you guys won't have to deal with the docs.

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 this pull request may close these issues.

2 participants