This is a placeholder to track the port of the Security Kit module to Drupal 8. I don't see any 8.x branch for now. Maybe it would worth starting one for people to start contributing?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jweowu’s picture

Sure. Branched & pushed. Go nuts! :)

Anonymous’s picture

Cool, I would like to help on that.

Anonymous’s picture

FileSize
81.95 KB

I and some friends have done some work on D8 for seckit. There is still a validation to port and 2 tests, but the other validations passed the simpletest!!!
People who contributed to this patch:

luizsgpetri
kadubr
tzambotti

jweowu’s picture

Status: Active » Needs review

That sounds like an excellent start. Nice work!

I don't want to get involved in the D8 side yet, but if someone would like to confirm that this is a good basis for continuing efforts, then I'll be happy to commit this to the 8.x branch, so that the subsequent patches can be smaller.

anavarre: perhaps you are in a position to provide confirmation?

n.b. I'm setting this to "Needs Review" for the above reason, but with the expectation that we'll bump it back to "Active" after the initial commit.

anavarre’s picture

@jweowu: this port was part of a D8 hackathon (more info here) with the folks at CI&T and @typhonius and @hernani from Acquia so I trust the overall port to go in the right direction. I'll get in touch with @typhonius to discuss that more.

@luizsgpetri: I see a fatal when enabling the module:

PHP Fatal error: Call to undefined function get_t() in /var/www/html/head/modules/seckit/seckit.install on line 24

This is because get_t() is deprecated in Drupal 8.

When I remove line 24 and thus any reference to get_t() I can successfully enable the module, visit the permissions and configure pages just fine. It doesn't seem to cause any issue for now but any reference to variable_get/set/del() (more here) will have to be removed as well.

Could you please adjust that?

anavarre’s picture

Status: Needs review » Needs work
Anonymous’s picture

Thanks for the review @anavarre, I'll update the install to get rid of the old t function.
I'll double check the use of variable_X in the module but I think that should be just a few or none of them already.

adammalone’s picture

Additional things I noticed with the patch initially...

  • drupal8-version.patch:45: new blank line at EOF.
  • seckit.info still exists
  • seckit.test still exists
  • anavarre's changes to t() above
Anonymous’s picture

@anavarre and @typhonius.

Please take a look in the new patch.

anavarre’s picture

@luizsgpetri the patch doesn't apply. At first glance it seems to be caused by a missing seckit.install file.

diff --git a/seckit.install b/seckit.install
deleted file mode 100644
index a6d41fa..0000000
--- a/seckit.install
+++ /dev/null
@@ -1,88 +0,0 @@

Also, when you post a new patch could you please post an interdiff for us to quickly review the changes?

Anonymous’s picture

@anavarre, that is strange because I'm actually deleting the install file. It shouldn't throw any errors. It applied locally with no errors with git apply -v
. I'll create a interdiff for this patch so we can easily review the changes.

jeqq’s picture

Status: Needs work » Needs review
FileSize
126.96 KB

Fix patch #9, add compatibility with Drupal 8 alpha14 and other fixies.

jweowu’s picture

Cross-referencing with #2606748: [seckit] Security Kit in the Drupal 8 Contrib Porting Tracker issue queue.

badjava’s picture

Updated the patch to get it working again with rc2:

  • Added JS library for settings form
  • Converted $form_state to use classed object
  • Added URL class to fix URL issues in settings form
  • Removed redundant seckit.settings.yml~ backup file

There is still plenty of work to do on this module including:

  • Figuring out what functionality, if any, has moved into core
  • Re-implement drupal_add_http_header() usage as it has been deprecated
jweowu’s picture

Thanks badjava.

This seems like a good time to reiterate my earlier comment:

if someone would like to confirm that this is a good basis for continuing efforts, then I'll be happy to commit this to the 8.x branch, so that the subsequent patches can be smaller.
badjava’s picture

@jweowu Yes, I think this is a good starting point to build on.

badjava’s picture

I just discovered that this patch will need to be committed in order to use this module on sites that use Composer. Can we get this committed soon?

  • jweowu committed 0a7f751 on 8.x-1.x authored by luizsgpetri
    Issue #2286055 by luizsgpetri, kadubr, tzambotti, anavarre, typhonius,...
jweowu’s picture

Committed.

luizsgpetri, I don't know why, but your posts in this issue are now listed as Anonymous? I could see that you're still active here, with commits in other Drupal projects, so I lifted the attribution details from one of those. I hope that's ok.

jweowu’s picture

Status: Needs review » Needs work

I'm pretty sure "Needs work" is still correct here.

It feels sensible to keep development in this issue until we're at a reasonably stable point? We can just hide the redundant files as things get committed along the way.

timmillwood’s picture

So now there's a 8.x-1.x can someone make a release so we can create issues against it.

I just got the following issue when running all tests against my install.

php ./core/scripts/run-tests.sh --verbose --keep-results --color --concurrency 30 --php `which php` --sqlite /tmp/test.sqlite --url http://localhost:8080 --all
exception 'Drupal\simpletest\Exception\MissingGroupException' with message 'Missing @group annotation in Drupal\seckit\Tests\SecKitCSPCaseTest' in /home/travis/build/timmillwood/d8/docroot/core/modules/simpletest/src/TestDiscovery.php:329
Stack trace:
#0 /home/travis/build/timmillwood/d8/docroot/core/modules/simpletest/src/TestDiscovery.php(162): Drupal\simpletest\TestDiscovery::getTestInfo('Drupal\\seckit\\T...', '/**\n * Function...')
#1 /home/travis/build/timmillwood/d8/docroot/core/modules/simpletest/simpletest.module(510): Drupal\simpletest\TestDiscovery->getTestClasses(NULL)
#2 /home/travis/build/timmillwood/d8/docroot/core/scripts/run-tests.sh(880): simpletest_test_get_all(NULL)
#3 /home/travis/build/timmillwood/d8/docroot/core/scripts/run-tests.sh(99): simpletest_script_get_test_list()
jweowu’s picture

I wasn't expecting to do that yet, but I'll defer to the people working on the code. If they'd prefer separate issues at this stage, we can do that; otherwise we'll continue to isolate the 8.x development in here.

Yury N’s picture

As D8 has deprecated drupal_add_http_header() function. I've created a patch for 8.x-1.x branch that utilizes Response object.

badjava’s picture

Patch applies cleanly for me and fixes the drupal_add_http_header() function deprecation issue.

With this patch we should have a functional D8 port. We need to get this patch committed and then a release cut so we can start filing individual issues against 8.x.

I am willing to help with ongoing support of this module so if someone wants to add me as a maintainer, I would be willing to take a more active role particularly with the D8 version.

timmillwood’s picture

Status: Needs work » Needs review

I'd second @badjava to be co-maintainer.

dabito’s picture

I second the move for a 8.x release so we can get this port going!

jweowu’s picture

FYI I'm on holiday for another week and a bit, and have extremely limited access to anything in the meantime. Please continue to create patches if you have improvements, and I will follow up with commits and a release once I'm back. Thanks!

  • jweowu committed 8798463 on 8.x-1.x authored by YurikK_
    Issue #2286055: by YurikK_, badjava, et al: Port to Drupal 8
    
    Use...
jweowu’s picture

Status: Needs review » Closed (fixed)

I've created a 8.x-1.x-dev release after that last commit. It should be visible shortly. Marking this issue as Fixed.

jweowu’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev