Skip to content

Add null attribute check in cloneResultMap method #16735

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

Merged
merged 11 commits into from
Apr 9, 2025

Conversation

mpadinhabrandao
Copy link

@mpadinhabrandao mpadinhabrandao commented Apr 1, 2025

Hello!

  • Type: bug fix
  • Link to issue: #16736
  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

Small description of change:

when type model and use this on leftjoin I have error when the leftjoin line doesn't exists because try assign null in variable that aren't null

Thanks

@raicabogdan
Copy link
Contributor

@mpadinhabrandao Need to create an issue first explaining the issue you have, link that issue into your pull request and also update the change log. The target needs changed to 5.0.x, the master target is only for releases.

@niden
Copy link
Member

niden commented Apr 1, 2025

@mpadinhabrandao could you please add a test for this?

@mpadinhabrandao mpadinhabrandao changed the base branch from master to 5.0.x April 1, 2025 16:38
@mpadinhabrandao
Copy link
Author

@raicabogdan, I will create the issue later and update the description and the changelog.
@niden, After I create the issue I will try to make the test. Do you have any example?

@raicabogdan and @niden about the code, what is your opinion?

@raicabogdan
Copy link
Contributor

raicabogdan commented Apr 1, 2025

Can work I think, theoretically it just skips assignment on null and leaves the model as is. Its why I haven't exactly said no to your suggestion fix on discord. Its just that the cloneResult works in other places as well and as such I'm not sure if it will break anything. @niden may have other insights... but he may not be aware of the underlying discussion, thus an issue explaining the problem is recommended.

Le. Looks like it's breaking tests

@mpadinhabrandao
Copy link
Author

@raicabogdan and @niden, I have already written some tests and fixed the errors identified on the first try.
Who can repeat the actions?

@mpadinhabrandao
Copy link
Author

I already fixed the phpcs and tomorrow I am going to review the integration errors

@mpadinhabrandao
Copy link
Author

I already test with own product and solution works.
could you run the actition?

@mpadinhabrandao
Copy link
Author

@niden, @raicabogdan , can someone start the tests?

@raicabogdan
Copy link
Contributor

Sorry, but its not my decision, only @niden or @Jeckerson can do that.

@niden niden merged commit 23b883d into phalcon:5.0.x Apr 9, 2025
42 checks passed
@niden
Copy link
Member

niden commented Apr 9, 2025

Thank you @mpadinhabrandao

@niden niden added bug A bug report status: medium Medium 5.0 The issues we want to solve in the 5.0 release labels Apr 9, 2025
Copy link
Contributor

@rudiservo rudiservo left a comment

Choose a reason for hiding this comment

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

Squashing commits would be nice, rest seems ok and valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants