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

Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete respect parent api #1257

Merged
merged 6 commits into from
Oct 11, 2022
Merged

Conversation

midlan
Copy link
Collaborator

@midlan midlan commented Oct 11, 2020

Description (*)

I fixed overrides of Mage_Core_Model_Resource_Db_Abstract::delete to respect parent api, which means:

  • throw on error
  • return $this

Fixed Issues (if relevant)

  1. Fixes No throw when Mage_Admin_Model_User::delete fails #950

Manual testing scenarios (*)

Admin panel

  1. make some constraint for admin_user table that not allow you delete record (e.g. foreign key referencing the table with RESTRICT rule ON DELETE)
  2. go to admin panel and try delete some user, instead of green message that user was deleted, we should get red error message

API

There is changes also in API resource model, so we should test it also.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api Component: Core Relates to Mage_Core labels Oct 11, 2020
kkrieger85
kkrieger85 previously approved these changes Oct 20, 2020
@midlan
Copy link
Collaborator Author

midlan commented Mar 4, 2021

@Flyingmana @sreichel review pls?

@midlan midlan added the backwards compatibility Might affect backwards compatibility for some users label Aug 24, 2021
@midlan midlan changed the base branch from 1.9.4.x to 20.0 August 24, 2021 08:54
@midlan midlan dismissed kkrieger85’s stale review August 24, 2021 08:54

The base branch was changed.

@midlan
Copy link
Collaborator Author

midlan commented Aug 24, 2021

It is BC break, labeled it and changed base branch to 20.0

Sekiphp
Sekiphp previously approved these changes Oct 22, 2021
Copy link
Collaborator

@Sekiphp Sekiphp left a comment

Choose a reason for hiding this comment

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

seems ok

for future compatibility, you should consider to replace catch of Exception to Throwable

@midlan midlan added the review needed Problem should be verified label Dec 23, 2021
@fballiano
Copy link
Contributor

seems ok

for future compatibility, you should consider to replace catch of Exception to Throwable

I switched them

fballiano
fballiano previously approved these changes Jun 15, 2022
@fballiano
Copy link
Contributor

But we should still add a note to the README in the "differences between v19 and v20"

@elidrissidev
Copy link
Member

Updated the branch since the README's version was ancient and added the note to the README.

@elidrissidev elidrissidev changed the title Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete to respect parent api Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete respect parent api Oct 11, 2022
@elidrissidev elidrissidev removed the review needed Problem should be verified label Oct 11, 2022
@elidrissidev elidrissidev merged commit 61f1a22 into OpenMage:20.0 Oct 11, 2022
@github-actions
Copy link

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  7 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 61f1a22. ± Comparison against base commit bc4b2cd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards compatibility Might affect backwards compatibility for some users Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api Component: Core Relates to Mage_Core documentation hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No throw when Mage_Admin_Model_User::delete fails
7 participants