Closed (fixed)
Project:
Konami Code
Version:
7.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
23 Nov 2017 at 10:24 UTC
Updated:
30 Nov 2018 at 13:39 UTC
Jump to comment: Most recent
Comments
Comment #2
g33kg1rl commentedSome actions aren't working on https on my website as well.
Comment #3
bramdriesenI've checked and I'm afraid that for those actions there is not really a fix possible since some actions rely on external sites, which do not support HTTPS.
So there are 2 options:
- Disable/add mention that those specific actions only work on HTTP sites
- Remove them completely since we live in 2018 and almost every site has https
Comment #4
bramdriesenSome sites seem to support HTTPS, so I'm going to create some child issues to fix this in the D7 version. Others are just references to a JS file, so if we would include those in the module itself it should solve the issue.
Comment #5
jelle_sMaybe don't include them in the source code, but add a makefile that adds them as a library?
Comment #6
bramdriesenCould be me, but I don't really see how a makefile could solve this issue.
Comment #7
jelle_sWhen the module is downloaded with drush, it will automatically detect the makefile and download the library. See for example the plupload module: https://cgit.drupalcode.org/plupload/tree/plupload.make.example?h=7.x-2.x
IMHO it's better to keep libraries out of the repo. The git history will be much cleaner if the library needs to be updated (a diff in a url/version number, vs all changes to the library if it's included in the repo).
Comment #8
bramdriesenI like the idea!
Will this also work with Drupal.org packaging? (users downloading the .tar file?)
Comment #9
jelle_sNo, it downloads the file to sites/all/libraries/[library_name], so it can't be included in the .tar file. Best might be to create a INSTALL.txt or add the instructions to the README. See https://cgit.drupalcode.org/plupload/tree/README.txt for inspiration :)
You might need to add a dependency to the libraries module to make things easier (defining the js file as a library and adding it using the libraries module).
Comment #10
bramdriesenOkay makes sense. We should also take this into account: https://www.drupal.org/node/2947530. I'm not sure that all of the existing actions for the D7 meet this requirement... seems like we have quite some work to do for the D7 module to be more compliant with the general standards.
For the D8 port I'll take all of this into account from the start of course when porting existing actions that depend on external JS libraries.
Comment #11
jelle_sYeah it could be quite some work.
I'm not the maintainer of this module and I don't know how much free time you have, but for what it's worth, if I were the maintainer I would focus on just having the existing actions work for now. Since most people are probably moving to D8 anyway it might not be worth the effort to do a complete rewrite (in essence...). But I'll leave that decision up to you of course :)
Comment #12
bramdriesenYou're correct :) the D8 version should work according to the standards. But since D7 will be used less and less, not quite sure yet how I'm going to solve those issues for D7 :).
Comment #15
bramdriesenFixed them by replacing http with https. I don't feel like doing a complete rewrite for the D7 version to fix those dependencies.
One action won't work (geocities izer) so I added a "Note" on that action to describe the issue.