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

(fix) Check for rootPath property #8

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

curtishenson
Copy link
Contributor

Seems like not all file systems have the rootPath property.

We ran into an issue similar to #7 but with the servd assets plugin. Which is S3 under the hood.

Checking for rootPath and defaulting to an empty string seems to make everything work again.

I wasn't able to test a local filesystem to see if this caused any issues there.

Seems like not all file systems have the rootPath property
src/Folder.php Outdated Show resolved Hide resolved
@nitriques
Copy link
Member

Hey @curtishenson

Sorry for the late response.
Thanks for the fix! I do not use S3, but we do aim to be compatible with as many FS as possible.

I added 7fa52dc to your branch, it's purely cosmetic.

This will be release with craft 5 support. Let us know if you need that for craft 4, as it can be easily backported.

Thanks again !

@nitriques nitriques changed the title Check for rootPath property (fix) Check for rootPath property Aug 13, 2024
@nitriques nitriques merged commit a03694f into DeuxHuitHuit:dev Aug 13, 2024
1 check failed
@curtishenson
Copy link
Contributor Author

Awesome!

@pixelcab
Copy link

pixelcab commented Sep 6, 2024

Hey @curtishenson & @nitriques, I'm relatively new to CraftCMS, and I may simply be missing a configuration or env variables somewhere, but while using the Local fs and setting the craft-cloudflare-stream setting to use form input instead of public URL. The getAssetFolderPath method will only return null for the $path value, which would cause a failure in the uploadVideoByPath method.

After reverting the changes, this PR implemented (locally) the $path was able to be generated, minus the Volume subfolder (which I've removed for now), and the video was uploaded to CF Stream without issue.

This is a long-winded way of saying, either would you be able to point me in the direction of what config value I may need to include, or this change accidentally breaks Local FS.

Thanks in advance, and I appreciate your efforts here.

@nitriques
Copy link
Member

Hey @pixelcab !

I'm relatively new to CraftCMS, and I may simply be missing a configuration or env variables somewhere,

Welcome! We DO want beginners to be able to use the extension, so this is great !


We do use the local FS all the time with this plugin and it works pretty well on our end.

The getAssetFolderPath() function does not even know about the setting and should work exactly the same way in either case.

Could I ask you to use dd() to get the values of your root path ? The only way I see the $path variable being null is if your rootPath is null. AFAICT, the rootPath of the local FS will never be null (this may point to a different problem).

Also, could you provide the complete stack trace of the error you are seeing with the current code ? Thanks !

nitriques added a commit that referenced this pull request Sep 30, 2024
Re #8

This PR seems to be needed for craft 5
nitriques added a commit that referenced this pull request Sep 30, 2024
* (fix) Use path property instead of rootPath

Re #8

This PR seems to be needed for craft 5

* Format code
nitriques added a commit that referenced this pull request Oct 2, 2024
This is a backport of #8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants