pkp/pkp-lib#10668 Announcement image handling fixed #10787
Conversation
There was a problem hiding this comment.
Thanks, missed that when refactoring Announcements!
My main concern is why settings aren't removed together with the main Model (announcements). Is this some sort of edge case? Or maybe this was the case before this issue was resolved #10562?
| @@ -119,7 +119,7 @@ public function save(array $options = []) | |||
|
|
|||
| Hook::call('Announcement::add', [$this]); | |||
|
|
|||
| $hasNewImage = $this?->image?->temporaryFileId; | |||
| $hasNewImage = isset($this?->image?->temporaryFileId) ? $this->image->temporaryFileId : null; | |||
There was a problem hiding this comment.
I hoped PHP null safe operator does this automatically :(
There was a problem hiding this comment.
me too . But for simple stdClass object , PHP register waring also which is better to avoid .
There was a problem hiding this comment.
What about $hasNewImage = $this?->image?->temporaryFileId ?? null;
There was a problem hiding this comment.
What about
$hasNewImage = $this?->image?->temporaryFileId ?? null;
That should work .
| @@ -376,6 +376,11 @@ public function coerce($value, $type, $schema) | |||
| return $newArray; | |||
| case 'object': | |||
| $newObject = []; // we handle JSON objects as assoc arrays in PHP | |||
|
|
|||
| if (isValidJson($value)) { | |||
There was a problem hiding this comment.
Hmm, in which situation we need this?
There was a problem hiding this comment.
In our case. we are getting the data as JSON but here we try to access each inner prop of JSON data as array, it handle that case we for announcement files data are not converted to JSON to array .
| // need to delete the image for this announcement model instance | ||
| if (!$hasNewImage && !$this?->image && $this->fresh()->image) { | ||
| $this->deleteImage(); | ||
| DB::table($this->getSettingsTable()) |
There was a problem hiding this comment.
I would expect that settings are removed together with the main model, isn't this happening?
There was a problem hiding this comment.
This handle a case when at updating an announcement image is deleted and that is the only update .
There was a problem hiding this comment.
If it's the only case, I'm ok with it)
|
Thanks, for explaining. Looks good for merging then! |
touhidurabir
force-pushed
the
i10668_main
branch
from
3bdca93 to
69319c0
Compare
for #10668