Skip to content

Conversation

@ehhc
Copy link
Contributor

@ehhc ehhc commented Jan 26, 2019

Strange url-handling reverted + tests modified so that they might fin…d that issue in the future + test modified so that they both contain tests for posix and windows path separator -> fixes #2834

ehhc added 3 commits January 26, 2019 17:21
…d that issue in the future + test modified so that they both contain tests for posix and windows path separator -> fixes BoostIO#2834
@ehhc
Copy link
Contributor Author

ehhc commented Jan 26, 2019

the idea of the changes in the tests was that they should test both for posix and windows systems.. obviously that still dosen't work correct (they are green at my maschine since the fist commti -.-)

@daiyam
Copy link
Contributor

daiyam commented Jan 26, 2019

The problem with the regex '/?' + STORAGE_FOLDER_PLACEHOLDER + '.*?"' is the usage of the terminator ".

For an image, it generates <img src="<path>", so the path is correctly found.
For a gallery, it generates:

<div class="gallery">
path1
path2
path3
</div>

In that case, the paths won't be found.

@daiyam
Copy link
Contributor

daiyam commented Jan 26, 2019

@ehhc, here the fixed regex:

function fixLocalURLS (renderedHTML, storagePath) {
  /*
    A :storage reference is like `:storage/3b6f8bd6-4edd-4b15-96e0-eadc4475b564/f939b2c3.jpg`.

    - `STORAGE_FOLDER_PLACEHOLDER` will match `:storage`
    - `(?:(?:\\\/|%5C)[-.\\w]+)+` will match `/3b6f8bd6-4edd-4b15-96e0-eadc4475b564/f939b2c3.jpg`
    - `(?:\\\/|%5C)[-.\\w]+` will either match `/3b6f8bd6-4edd-4b15-96e0-eadc4475b564` or `/f939b2c3.jpg`
    - `(?:\\\/|%5C)` match the path seperator. `\\\/` for posix systems and `%5C` for windows.
  */
  return renderedHTML.replace(new RegExp('/?' + STORAGE_FOLDER_PLACEHOLDER + '(?:(?:\\\/|%5C)[-.\\w]+)+', 'g'), function (match) {
    var encodedPathSeparators = new RegExp(mdurl.encode(path.win32.sep) + '|' + mdurl.encode(path.posix.sep), 'g')
    return match.replace(encodedPathSeparators, path.sep).replace(new RegExp('/?' + STORAGE_FOLDER_PLACEHOLDER, 'g'), 'file:///' + path.join(storagePath, DESTINATION_FOLDER))
  })
}

I've forgot that the \w doesn't include the -...

Anyway, thanks for the updated tests.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Jan 27, 2019
@Rokt33r
Copy link
Member

Rokt33r commented Jan 28, 2019

@daiyam @ehhc Is this pr ready? If so, I'll review it today.

@ehhc
Copy link
Contributor Author

ehhc commented Jan 28, 2019

@daiyam can you merge my PR with your fixed regex? So that we have a working attachmentMangement with proper tests again? + can you include tests for your image gallery??

@Rokt33r the PR would fix the attachment problem but would break the gallery... Unfortunately i don't have time to fix both.. but i think @daiyam will? (hopefully..?)

@Rokt33r
Copy link
Member

Rokt33r commented Jan 28, 2019

@ehhc Awesome! If @daiyam responses late, I'll try to merge this pr and disable gallery temporarily.

@daiyam
Copy link
Contributor

daiyam commented Jan 28, 2019

@Rokt33r go ahead to merge this PR to fix the urgent bug.
I will do another PR with the regex fixed and some tests for the gallery.

@Rokt33r
Copy link
Member

Rokt33r commented Jan 29, 2019

@daiyam Okay, then I'll just merge this PR and release again without undoing gallery.

@Rokt33r Rokt33r added this to the v0.11.14 milestone Jan 29, 2019
@Rokt33r
Copy link
Member

Rokt33r commented Jan 29, 2019

I confirmed that it works on Windows. Btw, I've tried this on macOS and it seems that both v0.11.13 and this pr don't work on macOS.(This should be other problem I think)

@ehhc Do you think you can fix this too?

@ZeroX-DG Could you check that dragging an image to editor works on Linux?

@ZeroX-DG
Copy link
Member

Dragging the image into editor works fine for me on Linux both the current version and this PR. Seem that Linux is not affected by this bug.

@ehhc
Copy link
Contributor Author

ehhc commented Jan 29, 2019

@Rokt33r sorry, i cannot fix it for Mac because i don't have one and i have no idea what the problem might be.. Sorry

@Rokt33r are the jest-Tests (yarn jest) working on Mac?

@Rokt33r
Copy link
Member

Rokt33r commented Jan 29, 2019

are the jest-Tests (yarn jest) working on Mac?

@ehhc Yeah it works fine.

screen shot 2019-01-29 at 17 42 41

@ehhc
Copy link
Contributor Author

ehhc commented Jan 29, 2019

@Rokt33r then i have no idea whats wrong on mac.. sorry :(

@daiyam
Copy link
Contributor

daiyam commented Jan 29, 2019

@Rokt33r I already have the PR #2642 to fix the issue you are getting 😉

@daiyam
Copy link
Contributor

daiyam commented Jan 29, 2019

@ehhc @Rokt33r I've made the PR #2848 to fix the regex

@Rokt33r Rokt33r removed the awaiting review ❇️ Pull request is awaiting a review. label Jan 30, 2019
@Rokt33r Rokt33r merged commit 6960c8b into BoostIO:master Jan 30, 2019
@ehhc ehhc deleted the fix_broken_attachments branch January 30, 2019 07:26
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.

can not preview local pics in 0.11.13

4 participants