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.

Sign up for GitHub

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

Jump to bottom

Persistence::action() should be read only #949

Merged
merged 4 commits into from
Jan 4, 2022
Merged

Persistence::action() should be read only #949

merged 4 commits into from
Jan 4, 2022

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jan 2, 2022 •
edited
Loading

when records are updated via SQL, php hooks can not be executed thus bypassing possible business logic checks

also SQL update statements has limited SQL support, for ex. table alias is not supported

it looks there was no use of them at all (one use in atk4/ui demos/collection/multitable.php)

in the future, Persistence::action should be renamed to Persistence::toQuery or even to toSelectQuery, related with #690

@mvorisek mvorisek force-pushed the action_must_no_update branch from c5e57de to 224ef47 Compare January 3, 2022 00:02
@mvorisek mvorisek marked this pull request as ready for review January 3, 2022 00:04
@mvorisek mvorisek force-pushed the action_must_no_update branch from 224ef47 to afe9df8 Compare January 3, 2022 00:10
@mvorisek mvorisek removed the bug label Jan 3, 2022
@mvorisek mvorisek changed the title Persistence::action must be read only Persistence::action should be read only Jan 3, 2022
@mvorisek mvorisek force-pushed the action_must_no_update branch from afe9df8 to 5e034ee Compare January 3, 2022 00:17
@mvorisek mvorisek changed the title Persistence::action should be read only Persistence::action() should be read only Jan 3, 2022
@mvorisek mvorisek mentioned this pull request Jan 3, 2022
Merged
@mvorisek mvorisek added the RTM label Jan 4, 2022
@mvorisek mvorisek force-pushed the action_must_no_update branch 2 times, most recently from 6c38650 to 0925739 Compare January 4, 2022 01:56
@mvorisek mvorisek force-pushed the action_must_no_update branch from 0925739 to bb53e74 Compare January 4, 2022 01:58
@mvorisek mvorisek merged commit 3c33808 into develop Jan 4, 2022
@mvorisek mvorisek deleted the action_must_no_update branch January 4, 2022 02:03
@mvorisek mvorisek mentioned this pull request Jan 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgehristov georgehristov Awaiting requested review from georgehristov

@DarkSide666 DarkSide666 Awaiting requested review from DarkSide666

@abbadon1334 abbadon1334 Awaiting requested review from abbadon1334

@ibelar ibelar Awaiting requested review from ibelar

Assignees
No one assigned
Labels
BC-break RTM
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mvorisek