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

Remove deprecated #1892

Merged

Conversation

mringler
Copy link
Contributor

@mringler mringler commented Jul 9, 2022

I'm getting warnings because declarations of setPrimaryKey() do not match between extending classes:

PHP Warning: Declaration of Nivm\Model\Person\Base\Person::setPrimaryKey(?int $key = NULL): void should be compatible with Nivm\Model\Util\Base\ChangedBy::setPrimaryKey($pk): void in /srv/www/html/nivm/src/Nivm/Model/Person/Base/Person.php on line 2700

setPrimaryKey(?int $key = NULL): void vs
setPrimaryKey($pk): void

I am extending tables via concrete_inheritance behavior, and the parent table does not have a primary key. Apparently this leads to the parent class having a stub for setPrimaryKey(), which is defined differently than the function on the child class. Since the parent class has not PK declaration itself, and can be parent to tables with different PK types, it is not possible to insert a type hint there.

Fortunately, adding setPrimaryKey() to tables without PK has been marked as deprecated since 2011, so I think the problem can be easily solved by just removing that function.

@mringler
Copy link
Contributor Author

mringler commented Jul 9, 2022

Uff, test fail because of problem with test setup:

Error: dealerdirect/phpcodesniffer-composer-installer contains a Composer plugin which is blocked by your allow-plugins config. You may add it to the list if you consider it safe.
You can run "composer config --no-plugins allow-plugins.dealerdirect/phpcodesniffer-composer-installer [true|false]" to enable it (true) or disable it explicitly and suppress this exception (false)

Not sure where this comes from or how to handle it

@dereuromark
Copy link
Contributor

This is now solved, you can probably rebase

@mringler
Copy link
Contributor Author

mringler commented Aug 9, 2022

Oh, completely forgot about this. Merged in master, now tests run through

@dereuromark dereuromark merged commit 34b713e into propelorm:master Aug 9, 2022
@mringler mringler deleted the remove_deprecated_addSetPrimaryKeyNoPK branch September 29, 2023 22:20
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.

None yet

2 participants