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

IBX-7959: Added ContentInfo::getSectionId strict getter #348

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

ciastektk
Copy link
Contributor

@ciastektk ciastektk commented Mar 15, 2024

Question Answer
JIRA issue N/A
Type improvement
Target Ibexa version v4.6
BC breaks no

Added getSectionId method to ContentInfo VO to avoid using php magic to get the value.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

@Nattfarinn Nattfarinn requested a review from a team March 18, 2024 07:36
@patrickallaert
Copy link
Contributor

I agree on the content of this PR, but not on the targeted branch (4.6). Versions 4.6.0 and 4.6.1 have already been released. Dot releases should only contain fixes and not new deprecations! Also, the change of the type of a property can lead to code that are incompatible between 4.6.1 and future 4.6.x.
I would suggest that this change happens in 4.7 instead.

@webhdx
Copy link
Contributor

webhdx commented Mar 18, 2024

Hey @patrickallaert

Yes we understand this is not compliant with semver in some regards. Unfortunately we do not plan to release 4.7 as we've been working on the next major already. We need to introduce new deprecations in the current 4.6 release as we want to improve the codebase considerably in 5.0, despite inconveniences it creates for developers. This particular change is also a puzzle to a bigger project where we plan to introduce proper getters on our Value Objects to avoid relying on magic methods - we learned on multiple occasions it has serious performance implications. What's more - the type hint was always present in the doc block and if you haven't been sanitizing the value it could blow up in other places too.

Hope you can understand our decision.

ciastektk and others added 2 commits March 18, 2024 13:50
Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com>
@ciastektk ciastektk force-pushed the added-get-section-id-to-content-info branch from 89407db to 130f3d8 Compare March 18, 2024 12:58
Copy link

sonarcloud bot commented Mar 18, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

@alongosz alongosz changed the title Added ContentInfo::getSectionId IBX-7959: Added ContentInfo::getSectionId Mar 19, 2024
@alongosz alongosz changed the title IBX-7959: Added ContentInfo::getSectionId IBX-7959: Added ContentInfo::getSectionId strict getter Mar 19, 2024
@alongosz alongosz merged commit 1b54380 into 4.6 Mar 19, 2024
24 checks passed
@alongosz alongosz deleted the added-get-section-id-to-content-info branch March 19, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants