Skip to content

Improve BasicEntityPersister to be more flexible and cleaner#12087

Merged
greg0ire merged 3 commits intodoctrine:2.20.xfrom
mvorisek:improve_basic_entity_persister
Jul 30, 2025
Merged

Improve BasicEntityPersister to be more flexible and cleaner#12087
greg0ire merged 3 commits intodoctrine:2.20.xfrom
mvorisek:improve_basic_entity_persister

Conversation

@mvorisek
Copy link
Copy Markdown
Contributor

@mvorisek mvorisek commented Jul 29, 2025

Individual changes are explained in commit messages.

Extracted from reworked #8260.

@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Jul 29, 2025

The commit messages are helpful, good job 👍
Can you please wrap the commit message body of 53433da at 72 chars, as recommended in https://www.doctrine-project.org/contribute/index.html#crafting-meaningful-commit-messages, and configure your IDE to always to that? 🙏

mvorisek added 3 commits July 29, 2025 15:15
@mvorisek
Remove BasicEntityPersister::$insertSql cache property
98e7a53 
When the persister is extended to do a multi update, the caching is not
wanted. The impact is minimal as the CPU/time overhead per query is
much bigger and the prepared statement is not cached anyway.
@mvorisek
BasicEntityPersister: refactor $values variable into $placeholders
6f5ce1a 
The new variable name is much more clearer.
@mvorisek
Prefer strict empty-array comparison over empty() call
8c6419e
@mvorisek mvorisek force-pushed the improve_basic_entity_persister branch from 23d10ec to 8c6419e Compare July 29, 2025 13:15
@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Jul 29, 2025

done

@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Jul 29, 2025

🤔 according to #11208 you should be targeting 3.6.x, since this is an improvement.

@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Jul 29, 2025

I want to follow with some more fixes based on this, can it go as "legacy improvement", ie. into 2.21.x?

@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Jul 29, 2025

I think you could even stay on 2.20.x, given how small this change is, but will the fixes really be undeniably fixes, or are they more questionably fixes?

@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Jul 29, 2025

Let's keep the target then, I plan more minor fixes for the BasicEntityPersister.

SenseException
SenseException approved these changes Jul 29, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upcoming fixes need to be reviewed separately and may need to be done on another target branch.

@greg0ire greg0ire merged commit e005239 into doctrine:2.20.x Jul 30, 2025
74 checks passed
@greg0ire greg0ire added this to the 2.20.6 milestone Jul 30, 2025
@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Jul 30, 2025

Thanks @mvorisek !

@mvorisek mvorisek deleted the improve_basic_entity_persister branch July 30, 2025 12:30
@github-actions github-actions bot mentioned this pull request Aug 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greg0ire greg0ire greg0ire approved these changes

@SenseException SenseException SenseException approved these changes

@beberlei beberlei Awaiting requested review from beberlei

@derrabus derrabus Awaiting requested review from derrabus

Assignees

No one assigned

Labels

Improvement

Projects

None yet

Milestone

2.20.6

Development

Successfully merging this pull request may close these issues.

3 participants

@mvorisek @greg0ire @SenseException