Replace non-display uses of getField#22517
Replace non-display uses of getField#22517cconard96 wants to merge 3 commits intoglpi-project:mainfrom
Conversation
|
There are plenty of I'm not really against this change, but maybe it should be documented (and maybe that kind of methods should really be deprecated in the future). |
I did start with making those replacements here but the use of something besides "id" as the identifier is not always so clear and maybe it was required to get the "id" field specifically. When used with SQL queries, surely an "items_id" column should be set to the "id"? If it should just be the identifier, it isn't supported because the identifier may not be an integer like Dashboards which use a string "key". I don't think these edge cases affected any of the |
That was just a global remark (ofr me aswell); since the |
|
Tests and lint are failing. Also, a rebase is needed to fix conflicts. |
trasher
left a comment
trasher
left a comment
There was a problem hiding this comment.
Tests and lint must be fixed
cedric-anne
left a comment
cedric-anne
left a comment
There was a problem hiding this comment.
IMHO, getField() occurences in the NotificationTarget*::getDataForObject() methods was intended. They result in having N/A displayed in notifications rather than no value displayed at all.
cconard96
force-pushed
the
task/replace_getField
branch
from
c63bee4 to
8eaa0f4
Compare
| if ($locations->fields['comment']) { | ||
| $data['##ticket.location.comment##'] = $locations->fields['comment']; | ||
| } | ||
| if ($locations->getField('room')) { | ||
| $data['##ticket.location.room##'] = $locations->getField('room'); | ||
| if ($locations->fields['room']) { | ||
| $data['##ticket.location.room##'] = $locations->fields['room']; | ||
| } | ||
| if ($locations->getField('building')) { | ||
| $data['##ticket.location.building##'] = $locations->getField('building'); | ||
| if ($locations->fields['building']) { | ||
| $data['##ticket.location.building##'] = $locations->fields['building']; | ||
| } | ||
| if ($locations->getField('latitude')) { | ||
| $data['##ticket.location.latitude##'] = $locations->getField('latitude'); | ||
| if ($locations->fields['latitude']) { | ||
| $data['##ticket.location.latitude##'] = $locations->fields['latitude']; | ||
| } | ||
| if ($locations->getField('longitude')) { | ||
| $data['##ticket.location.longitude##'] = $locations->getField('longitude'); | ||
| if ($locations->fields['longitude']) { | ||
| $data['##ticket.location.longitude##'] = $locations->fields['longitude']; | ||
| } | ||
| if ($locations->getField('altitude')) { | ||
| $data['##ticket.location.altitude##'] = $locations->getField('altitude'); | ||
| if ($locations->fields['altitude']) { | ||
| $data['##ticket.location.altitude##'] = $locations->fields['altitude']; | ||
| } | ||
| if ($locations->getField('address')) { | ||
| $data['##ticket.location.address##'] = $locations->getField('address'); | ||
| if ($locations->fields['address']) { | ||
| $data['##ticket.location.address##'] = $locations->fields['address']; | ||
| } | ||
| if ($locations->getField('postcode')) { | ||
| $data['##ticket.location.postcode##'] = $locations->getField('postcode'); | ||
| if ($locations->fields['postcode']) { | ||
| $data['##ticket.location.postcode##'] = $locations->fields['postcode']; | ||
| } | ||
| if ($locations->getField('town')) { | ||
| $data['##ticket.location.town##'] = $locations->getField('town'); | ||
| if ($locations->fields['town']) { | ||
| $data['##ticket.location.town##'] = $locations->fields['town']; | ||
| } | ||
| if ($locations->getField('state')) { | ||
| $data['##ticket.location.state##'] = $locations->getField('state'); | ||
| if ($locations->fields['state']) { | ||
| $data['##ticket.location.state##'] = $locations->fields['state']; | ||
| } | ||
| if ($locations->getField('country')) { | ||
| $data['##ticket.location.country##'] = $locations->getField('country'); | ||
| if ($locations->fields['country']) { | ||
| $data['##ticket.location.country##'] = $locations->fields['country']; | ||
| } |
There was a problem hiding this comment.
IMHO, for these lines, the solution is to do the same way as in line 350:
if ($locations->getFromDB($item->fields['locations_id'])) {
$data['##ticket.location.comment##'] = $locations->getField('comment');
// ...
}| = $row['problems_id']; | ||
| $tmp['##problem.date##'] | ||
| = $problem->getField('date'); | ||
| = $problem->fields['date']; |
There was a problem hiding this comment.
This one should probably be converted.
| = $problem->fields['date']; | ||
| $tmp['##problem.title##'] | ||
| = $problem->getField('name'); | ||
| = $problem->fields['name']; |
There was a problem hiding this comment.
| = $problem->fields['name']; | |
| = $problem->getField('name'); |
| ); | ||
| $tmp['##problem.content##'] | ||
| = $problem->getField('content'); | ||
| = $problem->fields['content']; |
There was a problem hiding this comment.
| = $problem->fields['content']; | |
| = $problem->getField('content'); |
| ->onlyMethods(['getPasswordExpirationTime']) | ||
| ->getMock(); | ||
| $user->method('getPasswordExpirationTime')->willReturn($expiration_time); | ||
| $user->getEmpty(); |
3 participants
Checklist before requesting a review
Description
CommonDBTM::getFieldis not meant to be used except for values that will displayed to the user. Maybe due to its name making it look like a simple getter, there are a lot of cases where the result of the function is used for SQL queries and other backend logic.