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

update updates to use propel orm #6867

Merged
merged 1 commit into from
Mar 8, 2024
Merged

update updates to use propel orm #6867

merged 1 commit into from
Mar 8, 2024

Conversation

DAcodedBEAT
Copy link
Contributor

Description & Issue number it closes

Screenshots (if appropriate)

How to test the changes?

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@DAcodedBEAT DAcodedBEAT added Code Smell php Pull requests that update Php code Security Platform: Database labels Feb 15, 2024
@DawoudIO
Copy link
Contributor

Looks good so far

@DAcodedBEAT
Copy link
Contributor Author

@DawoudIO / @MrClever I'm thinking of merging this after the next release just to not rock the boat too much. Thoughts?

@DawoudIO
Copy link
Contributor

Ya that sounds good

This was linked to issues Feb 20, 2024
Security Bug: SQL Injection - Event Editor via List Events #6849
Closed
Security Bug: SQL Injecton - Event Editor #6854
Closed
@DAcodedBEAT DAcodedBEAT added this to the vNext milestone Feb 20, 2024
grayeul
grayeul reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@grayeul grayeul left a comment

Choose a reason for hiding this comment

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

I'm not familiar with a lot of this, but I did look over it and added some comments/questions.

src/EventEditor.php
->setText(InputUtils::filterHTML($sEventText))
->setStart(InputUtils::legacyFilterInput($sEventStart))
->setEnd(InputUtils::legacyFilterInput($sEventEnd))
->setInActive(InputUtils::legacyFilterInput($iEventStatus));
Copy link
Contributor

Choose a reason for hiding this comment

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

less important, but it seems in this case it should be setInactive, as the 'In' is not a fully separate word.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, just using what was already set up to not scope creep - https://github.com/ChurchCRM/CRM/blob/master/propel/schema.xml#L506

Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted to open an Issue to potentially change the schema, to have consistent camelCase .... should we consider an additional TEMPLATE type in ISSUE_TEMPLATES? Maybe something like CodeCleanup? This wouldn't really be a bug, though I guess it could be argued to be a feature -- even though it shouldn't have any impact.

Also, do you know offhand if updating the schema (with name-format changes) would have any ripple effects? Or should everything just be properly auto-generated from that? And I haven't tried to find out what actually does that code generation, but it looks like all the files in the model/ChurchCRM/Base dir are being generated, so hopefully just changing schema is adequate.

@DAcodedBEAT DAcodedBEAT marked this pull request as ready for review March 8, 2024 22:27
@DAcodedBEAT DAcodedBEAT changed the title draft: update updates to use propel orm update updates to use propel orm Mar 8, 2024
@DAcodedBEAT DAcodedBEAT modified the milestones: vNext, 5.7.0 Mar 8, 2024
@DAcodedBEAT DAcodedBEAT merged commit 5a6166f into master Mar 8, 2024
4 checks passed
@DAcodedBEAT DAcodedBEAT deleted the propel-for-updates branch March 8, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grayeul grayeul grayeul left review comments

@DawoudIO DawoudIO Awaiting requested review from DawoudIO

@MrClever MrClever Awaiting requested review from MrClever

Assignees
No one assigned
Labels
Code Smell php Pull requests that update Php code Platform: Database Security
Projects
None yet
Milestone
5.7.0
Development

Successfully merging this pull request may close these issues.

Security Bug: SQL Injecton - Event Editor Security Bug: SQL Injection - Event Editor via List Events
3 participants
@DAcodedBEAT @DawoudIO @grayeul