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

Maybe touch() and save() together are redundant (possible edge-case) #30

Closed
beatwiz opened this issue Oct 16, 2022 · 2 comments · Fixed by #31
Closed

Maybe touch() and save() together are redundant (possible edge-case) #30

beatwiz opened this issue Oct 16, 2022 · 2 comments · Fixed by #31
Labels

Comments

@beatwiz
Copy link

beatwiz commented Oct 16, 2022

Bug Description

Hey!

Ive been using Guest Entries with success but I may have found a possible edge-case that could use some thought. Hear me out please.

Many thanks in advance for the excellent addon and for any insights!

Steps to reproduce

I have a listener set-up, listening for EntryCreated, so that when a File is attached to this field, it gets renamed and converted to JPG.

(...)
  if ($event->entry->presentation_file) {
      $name = 'presentation-' . $event->entry->title;
      $filename = 'abstracts/' . $name . '.' . $event->entry->presentation_file->extension();
  
      if ($event->entry->presentation_file->path() !== $filename) {
          Log::info($event->entry->presentation_file->path());
          Log::info($filename);
  
          if (Storage::disk('assets')->exists($filename)) Storage::disk('assets')->delete($filename);
  
          $new = $event->entry->presentation_file->rename($name); $filename
  
          if (strtolower($new->extension()) === "pdf") {
              $pdf = new Pdf($new->resolvedPath());
              $pdf->saveImage($new->resolvedPath() . '.jpg');
          }
      }
  }
(...)

It works fine on the CP.

Now, if i set up a Guest Entries, using file and save my attachment, the field becomes empty on the CP, despite the file was uploaded, renamed and even the JPG was processed. So the listener works, but in the end, the field is blank.

Heres what i found out:

The "culprit" was this: vendor/doublethreedigital/guest-entries/src/Http/Controllers/GuestEntryController.php @164

We have:

(...)
  // $entry->save();
  $entry->touch();
(...)

If I comment save(), everything works as expected. Hmmm.

While trying to dive in further....

Looking at: vendor/statamic/cms/src/Data/TracksLastModified.php @39

It seems that touch() also actually saves():

(...)
  public function touch($user = null)
  {
      $this->updateLastModified($user)->save();
  }
(...)

So I'm guessing having save() followed by touch() its either redundant and/or causes some bad behaviour on my listener by "double-triggering". But the funny thing is that if I actually log the actions on the Event Listener, they only happen ONCE even if I leave save() followed by touch().

What do you think? I hope that made any sense :)

Environment

Statamic 3.3.45 Pro
Laravel 8.83.25
PHP 8.0.24
anakadote/statamic-recaptcha 1.0.7
aryehraber/statamic-uuid 2.1.0
cnj/seotamic 2.1.0
doublethreedigital/guest-entries 1.2.2
optimoapps/statamic-bard-text-align 1.0.2
rias/statamic-data-import 1.2.2

@beatwiz beatwiz added the bug label Oct 16, 2022
@beatwiz beatwiz changed the title Edge case: Maybe touch() not needed after save() Maybe touch() not needed after save() (possible edge-case) Oct 16, 2022
@beatwiz beatwiz changed the title Maybe touch() not needed after save() (possible edge-case) Maybe touch() and save() together are redundant (possible edge-case) Oct 16, 2022
@duncanmcclean
Copy link
Owner

Well noticed - we shouldn't be calling both methods!

@github-actions
Copy link

Released as part of v1.2.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants