Skip to content

Conversation

@DeepDiver1975
Copy link
Member

Description

The comment verb is user input and needs validation in length.

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@owncloud owncloud deleted a comment from update-docs bot Sep 1, 2023
throw new \InvalidArgumentException('Non-empty String expected.');
}
$verb = \trim($verb);
if (\mb_strlen($verb, 'UTF-8') > IComment::MAX_VERB_LENGTH) {
Copy link
Member

Choose a reason for hiding this comment

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

If this is correct, it will need a comment.

I see a "verb" column in the oc_comments table, which is where this data will be stored I guess. The DB length is in bytes, and I think the mb_strlen will count the chars. If we consider non-latin languages, 1 char can take 3-4 bytes, which means that the verb can take up to 64 chars or 256 bytes, so this won't fit in the DB.
In addition, I assume that the MAX_VERB_LENGTH is counted in bytes. If it's counted in multibyte chars, it needs to be documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

THX. You are absolutly right about this. I blindly copied the logic from message.

This whole db-length-byte madness only applies to MySQL only ... right?

Copy link
Member

Choose a reason for hiding this comment

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

https://www.postgresql.org/docs/current/datatype-character.html

SQL defines two primary character types: character varying(n) and character(n), where n is a positive integer. Both of these types can store strings up to n characters (not bytes) in length

https://docs.oracle.com/cd/A57673_01/DOC/server/doc/SCN73/ch6.htm#varchar2

The VARCHAR2 datatype stores variable-length character strings. When you create a table with a VARCHAR2 column, you specify a maximum column length (in bytes, not characters)

https://dev.mysql.com/doc/refman/8.0/en/char.html

The CHAR and VARCHAR types are declared with a length that indicates the maximum number of characters you want to store. For example, CHAR(30) can hold up to 30 characters.

It seems oracle is the only one counting bytes, not chars...

@DeepDiver1975 DeepDiver1975 force-pushed the fix/comments-verb-length branch from 0725814 to 0935954 Compare November 16, 2023 10:06
@DeepDiver1975 DeepDiver1975 force-pushed the fix/comments-verb-length branch from 0935954 to 9870862 Compare November 16, 2023 10:25
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@DeepDiver1975 DeepDiver1975 merged commit e27c516 into master Nov 16, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/comments-verb-length branch November 16, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants