fix: read attachments from the path they were written to - #12
Open
daraghscopey wants to merge 1 commit into
Open
fix: read attachments from the path they were written to#12daraghscopey wants to merge 1 commit into
daraghscopey wants to merge 1 commit into
Conversation
The storage path accessor looked up
`filament-better-mails.logging.attachments.root`, but the shipped config
only defines that key under `mails.`. The lookup returned null, so the
prefix vanished and every attachment resolved to `/{id}/{filename}`
instead of `mails/attachments/{id}/{filename}`.
Downloads threw UnableToRetrieveMetadata, and ResendMailAction failed the
same way through the file_data accessor. On PHP 8.4 the null also raised
a deprecation in rtrim().
The reader now uses the same key and default as the writer. The writer in
turn drops its own inline path construction and uses the accessor, since
the record is created before the file is written. Two independent lookups
of one key is what let the two sides drift; now there is a single
derivation. The disk comes off the stored record for the same reason.
Stored files need no migration; they were always written to the right
path.
Fixes basementdevs#11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11.
BetterEmailAttachment::getStoragePathAttribute()looked upfilament-better-mails.logging.attachments.root, but the shipped config only defines that key undermails.. The lookup returnednull,rtrim(null, '/')gave an empty string, and the prefix vanished:config(...)NULL"mails/attachments"/1/report.docxmails/attachments/1/report.docxmails/attachments/1/report.docxmails/attachments/1/report.docxBecause
config()returnsnullfor a missing key rather than raising, the mismatch was silent. Downloads threwUnableToRetrieveMetadata, andResendMailActionfailed the same way through thefile_dataaccessor. On PHP 8.4 the null also raised a deprecation inrtrim().Changes
BetterEmailAttachment.php— reads the same key as the writer, with the same'mails/attachments'default so a published config missing the key falls back rather than resolving to null.BeforeSendingMailListener.php— drops its inline path construction and uses the accessor. The record is created before the file is written, so$attachment->storage_pathis already available. Two independent lookups of one key is what allowed the drift; there is now a single derivation. The disk comes off the stored record for the same reason, so the value written to the row cannot disagree with the disk actually used.Existing files need no migration — they were always written to the correct path. Only the reader was wrong.
Tests
Five tests in
tests/Feature/Core/Models/BetterEmailAttachmentTest.phpdrive the real listener and read back through the model:UnableToRetrieveMetadatavia the download streamAll five fail on
5.xbefore the change. Full suite passes after (41 passed, 2 skipped — both pre-existing). Pint clean.Note that once writer and reader share one derivation, the round-trip tests can no longer detect a wrong key on their own, since both sides move together. The three tests pinning literal path values are what guard the behaviour, so they are not redundant despite the overlap.