Skip to content

Conversation

@favincen
Copy link
Contributor

Base information

Question Answer
Related to a SourceForge thead / Another PR / Combodo ticket? Combodo's partners documentation
Type of change? Enhancement

Objective (enhancement)

DBObject provides helpers like Set(), SetIfNul(), and Copy(), but misses CopyIfNull().
CopyIfNull() is documented and recommended by Combodo in its partners documentation, and is really useful for its purpose.
Documentation states a PR to add this helper would be welcomed, so here it is.

Proposed solution (bug and enhancement)

Add the missing helper method in iTop, as it is documented.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

  • mention this new API method in documentation
  • update partners documentation to explain how to use instead of how to add, and remove recommandation to make a PR

@jf-cbd jf-cbd moved this from Hacktoberfest to First review needed in Combodo PRs dashboard Nov 3, 2025
@jf-cbd jf-cbd changed the title feat(DBObject): Add CopyIfNull() helper to copy attribute only if target is null N°8891 - feat(DBObject): Add CopyIfNull() helper to copy attribute only if target is null Nov 6, 2025
@jf-cbd
Copy link
Member

jf-cbd commented Nov 28, 2025

Hey @favincen, thank you for your PR. It has been functional accepted - which is normal given the content of the documentation 😁 We'll review it from a technical point of view

@jf-cbd jf-cbd moved this from First review needed to Pending technical review in Combodo PRs dashboard Nov 28, 2025
Co-authored-by: Thomas Casteleyn <[email protected]>
return $this->Copy($sDestAttCode, $sSourceAttCode);
}
$this->SetIfNull($sDestAttCode, $this->Get($sSourceAttCode));
return true;
Copy link
Contributor Author

@favincen favincen Dec 8, 2025

Choose a reason for hiding this comment

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

Note to Combodo: Set() and Copy() do return true unconditionaly upon completion, but SetIfNul() do not return any value. Seems unconsistant to me.
So how do this new CopyIfNull() method should behave concerning returning value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And should SetIfNull() be updated to return a boolean upon completion ?

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

The doc needs to be updated a bit, but over-all its ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Pending technical review

Development

Successfully merging this pull request may close these issues.

3 participants