Closed (fixed)
Project:
Image Widget Crop
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
9 Dec 2015 at 13:19 UTC
Updated:
15 Sep 2016 at 07:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
woprrr commentedNow we can start the review of this issue :) Thanks @lukas. We can specify all taks we needed on this refactor ?
Comment #3
luksakSince @sasanikolic wrote the code for handling libraries, he would be the guy to talk to. The main issue was how the libraries module works currently.
Comment #4
miro_dietiker@sasanikolic is just about completing his internship time at MD Systems and needs to focus on other projects issues.
Comment #5
luksakOk. I remember that @sasanikolic wrote a lot of code for handling one CSS and one JS file. This logic should be handled by libraries. Here is the code we are talking about:
image_widget_crop.module:
src/Form/CropWidgetForm.php:
Comment #6
ckaotikIt is also currently only possible to use the files from either a remote url (e.g. CDN) or the libraries module. Using local files without the libraries module, via relative paths such as `/libraries/cropper/cropper.min.js`, is not possible.
Since libraries has no D8 release, this is a dealbreaker for us since we must use local files due to privacy implications and want the config to work on all systems alike.
Comment #7
woprrr commentedYes ckaotik, i aggree with u ! We need to prevent all cases. What you suggest about it ?
"An module interface with checkbox 'CDN & EXTERNAL URL' and 'Local respository & Library module' to choose the method."
Comment #8
miro_dietikerIMHO it's not worth adding more complexity to support custom stuff without the libraries module.
Instead we should help make sure the libraries module is provide a stable release.
We could not afford maintaining library code duplication in all the modules where we build of libraries.
But sure, we should minimise code complexity as much as possible as proposed.
Comment #9
ckaotik@miro_dietiker I agree on getting the libraries module up and running, but i find it important to either fully depend on libraries being present or retain full compatibility with core (in addition to optionally libraries).
The module does not fully commit to either approach at the moment ;)
@woprrr I don't think additional UI is necessary. Couldn't we just use a priority logic such as this:
1) is `libraries` present? If so, use that to determine files.
2) does `file_exists` work on the url? If so, use the local files.
3) can we `curl` the url? If so, use the remote files.
4) No success? Use the CDN files.
Using a local file path already fails on form validation
parse_url($library_url, PHP_URL_HOST) && parse_url($library_url, PHP_URL_PATH)but might otherwise work. (haven't tested that)Comment #10
miro_dietiker@ckaotik We have many cases where we simply provide a trivial fallback if a soft dependency isn't met.
Almost no one is really depending to token. But the UX to browse tokens is greatly limited if you don't have it.
I don't see why we need to depend to libraries when we can provide a minimum fallback easily.
We could though display a warning that we strongly recommend the module in our settings.
Comment #11
ckaotik@miro_dietiker This is why I also suggested keeping support for both variants: libraries and core, but add support for local files while we're at it.
I'll try to find some time to propose a patch, though it may be a few days.
These (+ privacy concerns) are the reasons why we use local files.
Comment #12
miro_dietikerWe are fully aware of the CDN disadvantages. We still add it to the modules intentionally.
It lowers the barriers and makes them work out of the box and is convenience for a demo setup and finally makes even testing easier.
Privacy concerns and closed / local networking is why the advanced setting exists.
I'm looking forward to your proposal, but keep in mind, the issue is about simplification of code, not adding more cases and complexity.
If it is going to add complexity, it should go into a separate issue.
Comment #13
ckaotikI've attached a patch that should both reduce complexity and at the same time clear up the file priorities. This variant also allows to provide paths to local files.
The priorities are as follows (I hope I've correctly identified and retained their order):
1) Explicitly configured files will always be used.
2) If Libraries API is available and the Cropper library is installed, use those files.
3) Otherwise, use the CDN.
There are two unexpected things I ran into:
First, I had to remove the warning in `src/Element/ImageCrop.php` as the changed code will always provide a library that can be used (as long as the last fallback, the CDN, works). This should not create any issues.
Second, the Cropper library registration in `hook_libraries_info` expects the files in the library root (e.g. `libraries/cropper/cropper.min.js`). This does not match the structure of the packaged files, which places the script & style sheets in the `/dist/` sub-folder. As this also effects backward compatibility for users that placed the files in the expected path, I have not yet addressed this issue.
Comment #14
ckaotikFixed a minor inconsistency in detection of local paths.
Comment #15
ckaotikDamnit, it's too warm here. Third time's the charm.
Comment #16
woprrr commentedThank a lot for that patch :) !! I review it fast when i come back to my mission (thuesday).
Comment #17
miro_dietikerHow much test coverage do we have for these settings and error message cases?
IMHO it's an important piece to setup with enough complexity to argue all major cases should be covered. Otherwise our tests do not guarantee that all of the relevant cases are working cleanly.
Comment #18
ckaotikI would feel a lot safer with accurate tests, as the code changes are far from minor. To my shame I have to admit I've not yet worked with tests, so I can't help with that, sorry.
Comment #19
woprrr commentedDon't worry @ckaotik, i can finish this part (test). It not shame you patch is already an beautifull job ! I m glad to see your help.
@miro it's true DO add test coverage for this part ! I assign to me this part ;)
Comment #26
ckaotikFixed typo (
$csssinstead of$css) causing test fails. No interdiff because it's a minor change,Comment #27
woprrr commentedI switch to RTBC :) works a charm thanks all. To Tests part i decide to make this in a major issue a part assign to me.
Comment #29
woprrr commented