iframe-proxy

objecttothis · 2025-04-21T07:46:05Z

Migrate Sessions table to prevent problems with migrating populated databases.
Refactor a few function names for PSR-12 compliance.
Fix bug in migration causing failure due to trying to drop indexes with associated foreign key constraints. This bug was causing 3.3.2 databases to fail migration. [Bug]: Migration fails adding foreign key constraint #4202
migration to change constraint back to ON DELETE RESTRICT
20250213000000_fix_attributes_cascading_delete.php reverted a change on the ospos_attribute_links_ibfk_1 and ospos_attribute_links_ibfk_2 constraints on the ospos_attribute_links table to fix a bug with not being able to delete a dropdown value. The problem is that these need to be ON DELETE RESTRICT in order to use generated_unique_column and attribute_links_uq3 to enforce no duplication of attribute links. The solution to this (in this PR) is to change them back to ON DELETE RESTRICT and programmatically cascade delete before trying to remove an entry. This way we can enforce uniqueness AND deleting dropdown rows.
Add indices to ospos_inventory and ospos_attribute_links which were causing the items view queries to time out when attribute_links was large and the hardware wasn't fast.

closing #4237
closing #4251

- Function to delete attribute links with one or more attribute definitions. - Call function to implement an effective cascading delete. - Refactor function naming to meet PSR-12 conventions Signed-off-by: objecttothis <objecttothis@gmail.com>

- Add drop of Generated Column to prevent failure of migration on MySQL databases. Signed-off-by: objecttothis <objecttothis@gmail.com>

- Removed blank lines - Refactored function naming for PSR compliance - Reformatted code for PSR compliance - Added logic to drop dependent foreign key constraints before deleting an index then recreating them. Signed-off-by: objecttothis <objecttothis@gmail.com>

- DROP and CREATE session table to prevent migration problems on populated databases Signed-off-by: objecttothis <objecttothis@gmail.com>

- In the event that item_id = null (e.g., it's a dropdown) it should not be included in the results. Signed-off-by: objecttothis <objecttothis@gmail.com>

- Removed delete_value function in Attributes Controller as it is unused. - Renamed postDelete_attribute_value function for PSR-12 compliance. - Renamed delete_value Attribute model function for PSR-12 compliance. - Refactored out function to getAttributeIdByValue - Replaced == with === to prevent type juggling - Reorganized parts of model to make it easier to find CRUD functions. Signed-off-by: objecttothis <objecttothis@gmail.com>

- PSR-12 Compliance formatting changes - Refactored several generic functions into the migration_helper.php - First check if primary key exists before attempting to create it. - Grouped functions together in migration_helper.php - phpdoc commenting functions Signed-off-by: objecttothis <objecttothis@gmail.com>

- There are two queries run while opening the Items view which time out on large databases with weak hardware. These indices cut the query execution in half or better. Signed-off-by: objecttothis <objecttothis@gmail.com>

- This migration reverts ospos_attribute_links_ibfk_1 and 2 to ON DELETE RESTRICT. Cascade delete is done programmatically. This is needed to have a unique column on the attribute_links table which prevents duplicate attributes from begin created with the same item_id-attribute_id-definition_id combination Signed-off-by: objecttothis <objecttothis@gmail.com>

Signed-off-by: objecttothis <objecttothis@gmail.com>

objecttothis · 2025-05-22T11:50:52Z

        return $builder->get()->getResultArray();
    }
+
+// ==============


This heading would be for private Model functions.

I'd try to code to structure things logically.. having to add comments might be a code smell (according to clean code guidelines). It might just indicate naming is not good or classes are not sticking to single resonsibility principle.

objecttothis · 2025-05-22T11:53:26Z

@jekkos, I think the commit history is a little rough after rebasing on the master. When I merge, per usual I'm planning to do a squash and merge with a commit message of this to clean everything up:

- Refactored function names for PSR-12 compliance
- Programmatically cascade delete attribute_link rows when a dropdown attribute is deleted but leave attribute_link rows associated with transactions.
- Added `WHERE item_id IS NOT NULL` to migration to prevent failure on MySQL databases during migration
- Retroactive correction of migration to prevent MySQL databases from failing.
- Refactored generic functions to helper
- Reverted attribute_links foreign key to ON DELETE RESTRICT which is required for a unique constraint on this table. Cascading deletes are now handled programmatically.
- Migration Session table to match Code Igniter 4.6
- Add index to attribute_links to prevent query timeout in items view on large databases
- Added overridePrefix() function to the migration_helper. Any time QueryBuilder is adding a prefix to the query when we don't want it to, this query can be used to override the prefix then set it back after you're done.
- Added dropAllForeignKeyConstraints() helper function.
- Added deleteIndex() helper function.
- Added indexExists() helper function.
- Added primaryKeyExists() helper function.
- Added recreateForeignKeyConstraints() helper function.
- Added CRUD section headings to the Attribute model.
- Replaced `==` with `===` to prevent type juggling.
- Removed unused delete_value function.
- Reworked deleteDefinition() and deleteDefinitionList() functions to delete rows from the attribute_links table which are associated.
- Added deleteAttributeLinksByDefinitionId() function

This PR is now ready for review.

objecttothis · 2025-05-23T10:57:27Z

@jekkos the PHPLinter is failing on PHP 8.4 deprecations (https://github.com/opensourcepos/opensourcepos/actions/runs/15184079188/job/42700305258). I do think we need to make these changes to remove deprecated code, but that needs to be a separate PR from this one.

jekkos · 2025-05-27T21:30:34Z

We can keep the full history in the squashed commit message body. So typically a good commit message has a short first line describing what is is about, and then more details can be added after the first newline.

I find it very useful to add info to the history this way, it's a bit of an ongoing documentation with every change that is done.

jekkos

I'm not going to block the PR here at this point, I know these things take time to test. But I do have some minor remarks on the usage of comments and eventual removal of a duplicated log line. Beside that I think it looks good.

jekkos · 2025-05-28T22:11:59Z

        return $builder->get()->getResultArray();
    }
+
+// ==============


I'd try to code to structure things logically.. having to add comments might be a code smell (according to clean code guidelines). It might just indicate naming is not good or classes are not sticking to single resonsibility principle.

objecttothis · 2025-05-28T22:25:05Z

@jekkos in the morning I'll remove the comments and extra log lines. I think I was just mimicking what was in other migrations.

I'm OK with just ordering class functions logically, but I think we should agree on an order and move functions to those locations as we work on the code. Doing it all here would make the commit pretty difficult to parse.

I'll squash and merge after pushing those commits.

objecttothis · 2025-05-28T22:36:37Z

- Removed Comments separating sections of code in Attribute model - Removed extra log line to prevent cluttering of the log Signed-off-by: objecttothis <objecttothis@gmail.com>

objecttothis self-assigned this Apr 21, 2025

objecttothis linked an issue Apr 25, 2025 that may be closed by this pull request

[Bug]: Migration fails adding foreign key constraint #4202

Closed

1 task

jekkos reviewed Apr 29, 2025

View reviewed changes

Comment thread app/Database/Migrations/20240630000001_fix_keys_for_db_upgrade.php Outdated

jekkos reviewed Apr 29, 2025

View reviewed changes

Comment thread app/Database/Migrations/20240630000001_fix_keys_for_db_upgrade.php Outdated

jekkos reviewed Apr 29, 2025

View reviewed changes

Comment thread app/Database/Migrations/20250213000000_fix_attributes_cascading_delete.php

jekkos added this to the 3.4.1 milestone Apr 29, 2025

objecttothis requested a review from jekkos May 1, 2025 13:04

objecttothis marked this pull request as ready for review May 1, 2025 13:04

objecttothis commented May 1, 2025

View reviewed changes

Comment thread app/Controllers/Attributes.php

objecttothis commented May 1, 2025

View reviewed changes

Comment thread app/Database/Migrations/20210422000001_remove_duplicate_links.php

objecttothis commented May 1, 2025

View reviewed changes

Comment thread app/Database/Migrations/sqlscripts/3.4.1_attribute_links_unique_constraint.sql

objecttothis marked this pull request as draft May 1, 2025 13:20

jekkos requested changes May 1, 2025

View reviewed changes

Comment thread app/Controllers/Attributes.php

BudsieBuds reviewed May 2, 2025

View reviewed changes

Comment thread .editorconfig

jekkos reviewed May 12, 2025

View reviewed changes

Comment thread app/Views/attributes/form.php Outdated

objecttothis added 9 commits May 22, 2025 14:27

Implement Cascading Delete

03a6a60

- Function to delete attribute links with one or more attribute definitions. - Call function to implement an effective cascading delete. - Refactor function naming to meet PSR-12 conventions Signed-off-by: objecttothis <objecttothis@gmail.com>

Fix Migration

6e44336

- Add drop of Generated Column to prevent failure of migration on MySQL databases. Signed-off-by: objecttothis <objecttothis@gmail.com>

Fix Migration

c0263ba

- Removed blank lines - Refactored function naming for PSR compliance - Reformatted code for PSR compliance - Added logic to drop dependent foreign key constraints before deleting an index then recreating them. Signed-off-by: objecttothis <objecttothis@gmail.com>

Migrate ospos_sessions table

88d3d6f

- DROP and CREATE session table to prevent migration problems on populated databases Signed-off-by: objecttothis <objecttothis@gmail.com>

Fixed Bug in Migration

3d2c85c

- In the event that item_id = null (e.g., it's a dropdown) it should not be included in the results. Signed-off-by: objecttothis <objecttothis@gmail.com>

Optimizing Indices

6e38294

- There are two queries run while opening the Items view which time out on large databases with weak hardware. These indices cut the query execution in half or better. Signed-off-by: objecttothis <objecttothis@gmail.com>

objecttothis force-pushed the bugfix-constraint-migration-fix branch from 9c1dbce to b24dee4 Compare May 22, 2025 10:45

objecttothis marked this pull request as ready for review May 22, 2025 11:21

objecttothis requested review from BudsieBuds and jekkos May 22, 2025 11:21

Correct spacing after if for PSR-12

cae921d

Signed-off-by: objecttothis <objecttothis@gmail.com>

objecttothis commented May 22, 2025

View reviewed changes

Comment thread app/Models/Attribute.php Outdated

objecttothis commented May 22, 2025

View reviewed changes

objecttothis mentioned this pull request May 22, 2025

Fix item number lookup in sales/receivings (#4212) #4250

Merged

objecttothis commented May 28, 2025

View reviewed changes

Comment thread app/Database/Migrations/20250522000000_AttributeLinksUniqueConstraint.php

Comment thread app/Models/Attribute.php Outdated

jekkos previously approved these changes May 28, 2025

View reviewed changes

Minor code cleanup.

5584990

- Removed Comments separating sections of code in Attribute model - Removed extra log line to prevent cluttering of the log Signed-off-by: objecttothis <objecttothis@gmail.com>

objecttothis dismissed jekkos’s stale review via 5584990 May 29, 2025 11:20

objecttothis merged commit e1fedab into master May 29, 2025
3 checks passed

objecttothis deleted the bugfix-constraint-migration-fix branch May 29, 2025 11:24

This was referenced Apr 19, 2026

fix: Scope orWhere clauses in Item::exists() and Item::get_item_id() to fix items performance regression #4520

Merged

[Bug]: Items Performance after php8 upgrade #4519

Closed

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

objecttothis commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

objecttothis May 22, 2025

Choose a reason for hiding this comment

Uh oh!

jekkos May 28, 2025

Choose a reason for hiding this comment

Uh oh!

objecttothis commented May 22, 2025

Uh oh!

objecttothis commented May 23, 2025

Uh oh!

jekkos commented May 27, 2025

Uh oh!

Uh oh!

Uh oh!

jekkos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jekkos May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

objecttothis commented May 28, 2025

Uh oh!

objecttothis commented May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

objecttothis commented Apr 21, 2025 •

edited

Loading