fix(xpdo): Fix Illegal offset type for compound PK with generated field (#129)

[Ibochkarev]

Fix PHP warning "Illegal offset type" when saving objects with compound primary key that includes a generated field (e.g. AUTO_INCREMENT).

Problem
When getPK() returns an array for compound keys, the code used it as array index: $this->_fields[$this->getPK()] — PHP does not allow arrays as array keys.

Solution

• xPDOObject::save(): Iterate over PK fields, find the one with generated="native", and set only that field with getGeneratedKey()
• xPDOManager (MySQL): Extend getColumnDef to add AUTO_INCREMENT for generated fields in compound PK (only for the first column in PK, per MySQL requirement)

Additional changes

• Add NumberSeq test model and testSaveCompoundPkWithGeneratedField test
• Fix variable initialization in testGetObjectGraphsByPK / testGetObjectGraphsJSONByPK to avoid undefined variable notices
• Fix typo "retreiving" → "retrieving" in test messages

Fixes #129

[opengeek]

Thanks for this, @Ibochkarev! The fix is good. Tracing the "Illegal offset type" warning back to lastInsertId() returning an empty string on compound-PK objects with a generated field, and correcting the key assignment in save(), is the right approach.

I've pushed a commit on top before merging. I'm experimenting with a "fix-merge" approach on this project. By pushing small corrections directly onto contributor branches rather than asking for another round-trip, I'm hoping we can reduce friction and increase velocity on the project.

There are essentially two changes in the addendum:

1. Silent fallback replaced with a log warning.
After the generated key is assigned, your elseif block cleared _new even when lastInsertId() returned 0 — a zero-PK insert would silently look like a successful save. The addendum replaces that block with a LOG_LEVEL_WARN log call so callers get visibility into the failure. The success path (_new = false when a real ID comes back) is untouched.

2. Removed the pgsql and sqlite NumberSeq model files.
The pgsql and sqlite variants omit generated="native" on the key field, so they don't exercise this fix on those drivers. Keeping them would imply cross-driver coverage that doesn't exist. The addendum removes those six files and scopes this fix to MySQL. PostgreSQL (sequences) and SQLite (ROWID aliasing) both need manager-level changes — those belong in a follow-on PR.

Since this puts a commit on your branch that you didn't write, I'm flagging it before the merge proceeds. If anything in the addendum looks wrong or raises a concern, let me know.

[Ibochkarev]

Thanks for this, @Ibochkarev! The fix is good. Tracing the "Illegal offset type" warning back to lastInsertId() returning an empty string on compound-PK objects with a generated field, and correcting the key assignment in save(), is the right approach.

I've pushed a commit on top before merging. I'm experimenting with a "fix-merge" approach on this project. By pushing small corrections directly onto contributor branches rather than asking for another round-trip, I'm hoping we can reduce friction and increase velocity on the project.

There are essentially two changes in the addendum:

1. Silent fallback replaced with a log warning. After the generated key is assigned, your elseif block cleared _new even when lastInsertId() returned 0 — a zero-PK insert would silently look like a successful save. The addendum replaces that block with a LOG_LEVEL_WARN log call so callers get visibility into the failure. The success path (_new = false when a real ID comes back) is untouched.

2. Removed the pgsql and sqlite NumberSeq model files. The pgsql and sqlite variants omit generated="native" on the key field, so they don't exercise this fix on those drivers. Keeping them would imply cross-driver coverage that doesn't exist. The addendum removes those six files and scopes this fix to MySQL. PostgreSQL (sequences) and SQLite (ROWID aliasing) both need manager-level changes — those belong in a follow-on PR.

Since this puts a commit on your branch that you didn't write, I'm flagging it before the merge proceeds. If anything in the addendum looks wrong or raises a concern, let me know.

Hello! I don't have anything against this, as it speeds up the decision-making process regarding the PR and its integration into the release branch.