Skip to content

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

Merged
mvorisek merged 4 commits intodevelopfrom
action_must_no_update
Jan 4, 2022
Merged

Persistence::action() should be read only#949
mvorisek merged 4 commits intodevelopfrom
action_must_no_update

Conversation

@mvorisek
Copy link
Copy Markdown
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