Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
22 Jun 2019 at 22:05 UTC
Updated:
7 Jul 2019 at 19:29 UTC
Jump to comment: Most recent
Comments
Comment #2
westberliner commentedComment #3
bekirdag commentedHi,
There are 3 minor issues in total on the PAReview, 2 lines on the readme file, third is the console.warn on the JS file, all needs to be fixed.
After you fix those issues, you may want to fix the issues on https://www.drupal.org/pift-ci-job/1328477 for the release messages.
Thank you
Comment #4
bekirdag commentedComment #5
westberliner commentedComment #6
avpadernoComment #7
westberliner commentedHi,
yea, I thought that these are minor enough to skip them :)
I fixed them and I am still figuring out why my functional tests are not working under drupalci. Locally is everything fine. I disabled them for now.
See:
https://pareview.sh/pareview/https-git.drupal.org-project-jsnlog.git/rev...
https://www.drupal.org/pift-ci-job/1329096
Thx for reviewing.
Comment #8
westberliner commentedComment #9
klausiThanks for your contribution!
* why do you depend on the dblog module? It should not matter to which logging service you are writing to?
* The permission "use php for jsnlog visibility" is dangerous and should be marked as "restrict access". Any admin can execute arbitrary PHP code with that permission, so that needs to be made very clear. This is not a security blocker because you are only executing user provided PHP when the PHP module is enabled, which is good! I also don't even see how can enable PHP mode in the UI right now?
* You don't need to create a custom route access service. You can simply use _custom_access on your logging route. See https://www.drupal.org/docs/8/api/routing-system/access-checking-on-rout...
Otherwise looks good to me!
Comment #10
westberliner commentedHi,
You are right about dblog. I removed the dependency. Did not occurred to me during development.
The php snippet part removed as well. I was undecided - that is why it is have way in. But there are other and better ways for developers if complex logic is needed.
Regarding controller/access I like to keep the them separate. If it is alright and if there are no downsides to this method.
Commit: https://git.drupalcode.org/project/jsnlog/commit/ff63faf9898b4434bb878ff...
PAReview: https://pareview.sh/pareview/https-git.drupal.org-project-jsnlog.git
Tests: https://www.drupal.org/pift-ci-job/1329217
Thx for the review.
Comment #11
avpadernoThank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
I thank all the dedicated reviewers as well.