Skip to content

Conversation

@cderv
Copy link
Collaborator

@cderv cderv commented Feb 7, 2023

THIS PR IS AGAINST WINDOWS TESTS BRANCH

Local resources files won't be found as images/beach.jpg?bar is not a file. It errors on Windows (not on Linux).

Here I tweak test, as I don't think this is something we should really supports. Should we ?

I mean

![Image](images/beach.jpg?bar)

points a no real file and self contained will fail because image detected is not beach.jpg but beach.jpg?bar

If such local URL needs to be supported somehow, then we should parse resources by removing URL parameters.

Just want to check that before committing.

What are your thought @dragonstyle ?

Local resources files won't be found as `images/beach.jpg?bar` is not a file. It errors on Windows (not on Linux)
@dragonstyle
Copy link
Collaborator

It does seem like local paths do not need to support query params in this way- I think it's fine to remove this from the test. This does, to me, seem like a possible place to use safeExistsSync() since our point of view is that this couldn't be a valid file path (not that it should throw). Theoretically, someone could likely form a valid relative path to a file that will be present when deployed on a server which contains a query param which would be properly interpreted in the context of deployment (making the path a valid href, but not a valid local file path)...

@cderv
Copy link
Collaborator Author

cderv commented Feb 7, 2023

Theoretically, someone could likely form a valid relative path to a file that will be present when deployed on a server which contains a query param which would be properly interpreted in the context of deployment (making the path a valid href, but not a valid local file path).

yes that was my thinking too. This would work with Pandoc I guess only with self-contained: false right ?

Otherwise, Pandoc will try to insert something to encode and failed.

But I still think we should support - I'll fix with safeExistsSync()

cderv added 2 commits February 7, 2023 14:51
This reverts commit 50757a7 as we should try support this specific use case by fixing code instead.
@cderv
Copy link
Collaborator Author

cderv commented Feb 7, 2023

So I made the modification. I believe this will "solve" also #4194 (comment) by hiding the problem there.

The is the issue with safeExistsSync I guess, we'll hide so errors

@cderv
Copy link
Collaborator Author

cderv commented Feb 7, 2023

So I will merge this but it will hide the other issue. We just need to remember.

@cderv cderv changed the title Tweak test as local image with url parameter does not work Do not error on file existence when filtering resources Feb 7, 2023
@cderv cderv merged commit 8f87fbf into windows/run-tests Feb 7, 2023
@cderv cderv deleted the fix/vars-links branch February 7, 2023 14:57
@cderv cderv mentioned this pull request Feb 7, 2023
16 tasks
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