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 errors on Windows with paths to manifest #25

Closed

Conversation

engram-design
Copy link

I'm having issues with Formie (on Craft 5) where the paths are resolving incorrectly for many things, but mainly the manifest.json.

There are two parts to this PR:

Fix path to manifest
You may have to explain to me why you use FileHelper::createUrl() to get the path for the manifest file. It's a path, not a URL?

Windows:
C:\Users\joshcrawford\public_html\formie-craft5\vendor/verbb/formie/src/web/assets/forms/dist/manifest.json

MacOS
/Users/joshcrawford/public_html/craft50-alpha/vendor/verbb/formie/src/web/assets/forms/dist/manifest.json

You'll notice in Windows that the slashes are incorrect for a path, but correct if we assumed a URL, or if we assumed Linux/MacOS.

There might be other instances of FileHelper::createUrl() being used when dealing with paths.

Fix isAbsoluteUrl check
When it comes to fetching the manifest, you fetch it with curl. With the path sorted, that should be working fine, but an interesting result happens for some UrlHelper::isAbsoluteUrl($pathOrUrl) checks in your FileHelper.

UrlHelper::isAbsoluteUrl('/Users/joshcrawford/public_html/craft50-alpha/vendor/verbb/formie/src/web/assets/forms/dist/manifest.json');
false

UrlHelper::isAbsoluteUrl('C:\Users\joshcrawford\public_html\formie-craft5\vendor\verbb\formie\src\web\assets\forms\dist\manifest.json');
true

Here, you can see we input the MacOS path and the WIndows path respectively into this function, expecting the same result - but it's not! It's treating the C:\Users\... as an absolute URL, which it certainly isn't. I'm surprised that Craft/Yii don't check if the string you pass into it is a URL first, but that's probably unique to your use-case here, where you allow either a path or URL.

@khalwat
Copy link
Contributor

khalwat commented May 21, 2024

I'm going to consider this a CraftCMS issue until otherwise informed -> craftcms/cms#15043

@engram-design
Copy link
Author

Probably won’t fix 1387934 but happy to wait

@khalwat
Copy link
Contributor

khalwat commented May 22, 2024

@engram-design you may be right... I'm just super confused on how it could never have worked on Windows... because the code is the same for Craft 3 / 4 / 5 and surely someone would have complained by now, whether users of my plugins (or yours that use it).

I'll look at your change and maybe cherry-pick it if you think it's unrelated.

@khalwat khalwat reopened this May 22, 2024
@engram-design
Copy link
Author

Happy to wait for a resolution on craftcms/cms#15043 before going ahead!

khalwat added a commit that referenced this pull request May 22, 2024
khalwat added a commit that referenced this pull request May 22, 2024
khalwat added a commit that referenced this pull request May 22, 2024
@khalwat
Copy link
Contributor

khalwat commented May 22, 2024

Looks like they fixed (one of) the issues in Craft 4.9.5 and 5.1.5 -> craftcms/cms#15043 (comment)

I also pushed your normalizePath() fix in the above commits, I just moved it further down the chain to bottleneck it.

Can you update to the latest Craft, and then try it?

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-plugin-vite": "dev-develop as 1.0.35”,

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-plugin-vite": "dev-develop-v4 as 4.0.11”,

Then do a composer clear-cache && composer update

…..

Craft CMS 5:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-plugin-vite": "dev-develop-v5 as 5.0.1”,

Then do a composer clear-cache && composer update

@engram-design
Copy link
Author

Looks like that Craft fix alone has done the trick! Probably not a bad idea to normalize the path, but it's not seemingly needed on my end.

Thanks!

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.

2 participants