Skip to content

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Dec 2, 2024

For data object classes it is allowed to use _ in class names:

if (!preg_match('/[a-zA-Z][a-zA-Z0-9_]+/', $this->getName())) {
throw new Exception(sprintf('Invalid name for class definition: %s', $this->getName()));
}

But for object brick and field collection classes, this is currently not the case. From technical perspective this does not seem to have a valid reason - even for some features like column sorting / filtering in the grid, _ is not used as separator between brick and field name (instead there ~ is used which is fine because this is not a valid character for PHP class names).

So to be consistent, this PR adds support for _ also for object brick and field collection class names.

Note: I replaced [a-zA-Z0-9_] with \w in the regular expression because of your SonarCube rule:

Use concise character class syntax '\w' instead of '[a-zA-Z0-9_]'.

Copy link

github-actions bot commented Dec 2, 2024

Review Checklist

  • Target branch (11.4 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@BlackbitDevs BlackbitDevs force-pushed the bugfix/allow-underscore-in-object-brick-name branch from 50a6b88 to 473c3fe Compare December 2, 2024 13:02
@ghost ghost added the Pimcore:ToDo label Dec 3, 2024
Copy link

sonarqubecloud bot commented Dec 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Dec 3, 2024

You are right, @jdreesen - thanks. Let's see what SonarQube thinks now ;-)

Edit:

Enable the "u" flag or use a Unicode-aware alternative.

Imho it does not make sense to support unicode here because then also non-ASCII characters would be allowed. Have I overseen something?

@kingjia90
Copy link
Contributor

kingjia90 commented Dec 4, 2024

Imho it does not make sense to support unicode here because then also non-ASCII characters would be allowed. Have I overseen something?

Agree with you, it's not necessary and does not make sense.
I've marked the reported issues from SonarQube as false positive, so now it's all green 👍🏻

@alexz707 alexz707 changed the base branch from 11.4 to 11.5 December 17, 2024 11:30
@kingjia90 kingjia90 added this to the 11.5.5 milestone May 26, 2025
@kingjia90 kingjia90 added the Bug label May 26, 2025
@kingjia90 kingjia90 self-assigned this May 26, 2025
@kingjia90 kingjia90 merged commit 9647fe4 into pimcore:11.5 May 26, 2025
19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants