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

Fix remaining missing foreign key declarations #8333

Closed
12 tasks done
asmecher opened this issue Oct 14, 2022 · 7 comments · Fixed by pkp/ojs#3946, pkp/omp#1423, pkp/ops#543, #10023 or pkp/customBlockManager#86
Closed
12 tasks done
Assignees
Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day.
Milestone

Comments

@asmecher
Copy link
Member

asmecher commented Oct 14, 2022

This issue is a continuation of #6093.

The remaining missing foreign key constraints will require some code changes to move away from the use of 0 as a reserved case.

  • lib/pkp/classes/migration/install/CommonMigration.php:
    • site.redirect (renamed to redirect_context_id)
    • plugin_settings.context_id (uses 0 for site-wide)
    • notifications.context_id (uses 0 for site-wide)
    • notifications.user_id (uses 0 for "anyone" IIRC)
    • notification_subscription_settings (just needs the field to be renamed from context to context_id)
  • lib/pkp/classes/migration/install/LogMigration.php:
    • email_log.sender_id (uses 0 for "nobody in particular")
  • lib/pkp/classes/migration/install/MetadataMigration.php:
    • filters.context_id (uses 0 for site-wide)
    • filters.parent_filter_id (uses 0 for none)
  • lib/pkp/classes/migration/install/NavigationMenusMigration.php:
    • navigation_menus.context_id (0 used for site-wide)
    • navigation_menu_items.context_id (0 used for site-wide)
    • navigation_menu_items_assignments.parent_id (0 apparently used for none, even though the field appears nullable)
  • lib/pkp/classes/migration/install/classes/RolesAndUserGroupsMigration.php:
    • user_groups.context_id (0 used for side-wide)
      notification_subscription_settings

Some of these areas are slated for rewriting, so this work may be a lower priority than the rewrite.

It makes sense to keep the email_log even when a sender (user) is deleted, but it's probably needed to review GDPR (e.g. right to be forgotten), as the email might keep personal data.

@asmecher asmecher added the Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. label Oct 14, 2022
@asmecher asmecher added this to the 3.5 milestone Oct 14, 2022
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 7, 2023
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 7, 2023
@jonasraoni jonasraoni linked a pull request Jun 7, 2023 that will close this issue
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jun 7, 2023
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jun 7, 2023
@jonasraoni jonasraoni linked a pull request Jun 7, 2023 that will close this issue
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jun 7, 2023
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jun 7, 2023
@jonasraoni jonasraoni linked a pull request Jun 7, 2023 that will close this issue
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jun 7, 2023
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jun 7, 2023
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jun 7, 2023
@jonasraoni jonasraoni linked a pull request Jun 7, 2023 that will close this issue
@jonasraoni
Copy link
Contributor

I've updated the issue description (added the missing user_groups.context_id and marked others as done), and created initial PRs from detached commits of another issue.

jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 8, 2023
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 8, 2023
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 8, 2023
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 9, 2023
asmecher pushed a commit that referenced this issue Jun 9, 2023
* #9069 Added missing pre-foreign key cleanups

* #9069 Organized the migration code

* #9069 Added automated code while keeping the customizations

* #9069 Added code to consider the dependencies when running the entity cleanup

* #9069 Added code to ignore a couple of special values, which will be handled by later migrations

* #9069 Added code to dynamically ignore nonexistent entities (review_*)

* #9069 Moved cleanup of notifications.user_id to the foreign key migration and fixed typo

* #9069 Added code to drop existing foreign keys

* #9069 Updated cleanup to consider a database without the foreign keys from #8333

* #9069 Delegated cleanup of future foreign keys to #8333
@jonasraoni jonasraoni reopened this Jun 9, 2023
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Jun 9, 2023
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jul 24, 2024
jonasraoni added a commit to pkp/staticPages that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ojs that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/omp that referenced this issue Jul 24, 2024
jonasraoni added a commit to jonasraoni/ops that referenced this issue Jul 24, 2024
@asmecher
Copy link
Member Author

asmecher commented Aug 8, 2024

@jonasraoni, a tweak is required on this one: you're using ALTER TABLE t RENAME INDEX old_name TO new_name, which is only available in MariaDB as of version 10.5.2. My Ubuntu ships with 10.3.39 and I think there will probably be lots of ISPs using older versions too. Could you check if there's a workaround that'll support older MariaDBs?

@asmecher asmecher reopened this Aug 8, 2024
emmauhl added a commit to pkp/pkp-docs that referenced this issue Aug 14, 2024
…e-context

pkp/pkp-lib#8333 Update route for the site context on the API from "_" to "index"
@asmecher
Copy link
Member Author

This changeset renamed the notification_subscription_settings_context index to notification_subscription_settings_context_id -- just an index rename, no functional change. However...

  • The old index name remained in CommonMigration.php, and
  • Index renaming is not supported in MariaDB 10.x or older but supported versions of MySQL.

I've backed out the change. If we do decide to rename it, let's wait until we can set a baseline MariaDB/MySQL requirement that'll allow us to do it cleanly (e.g. without deleting and re-creating it, which will disturb foreign key constrants). It's just cosmetic.

asmecher added a commit that referenced this issue Sep 20, 2024
asmecher added a commit that referenced this issue Sep 20, 2024
@jonasraoni
Copy link
Contributor

I'll close this as the renameIndex() was dropped, if there are differences (regarding a new/upgraded installation) it might be handled better in another issue (e.g. #9250).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment