Here is a prototype developed from 7.x-2.x. It is somewhat functional--you can download, import, and query the Browscap data. I tried to make the minimum changes to both get it working and fit the mold of a D8 module.

It provides a new API:

  use Drupal\browscap\Browscap;
  Browscap::getBrowser($some_user_agent);

The thought is having an importable, camel-cased class method is more appropriate to D8. The old API, browscap_get_browser() is retained but deprecated. (Is this a good change?)

Major problems:
- Loading the data into the database is extremely slow. It took over one hour to complete on my local system, versus less than 30 seconds under D7. There seems to be a performance problem with the db_insert() and db_delete() calls.
- Lacks D7 update code.
- Automatic updates untested.
- Caching untested.

Improvements:
- Always uses gzip compression for downloads. (Drupal 8 requires PHP 5.4 or greater, which has gzip support.)
- Download URLs are configurable. (Incorporates #1788720-12: Allow to change the URLs to use for importing useragent information)

Please review!

CommentFileSizeAuthor
#17 2301335-port-browscap-to-d8-17.patch66.63 KBlahoosascoots
#1 browscap-port_to_d8-2301335-1.patch30.77 KBaohrvetpv
browscap-d8_port.patch29.1 KBaohrvetpv

Comments

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new browscap-port_to_d8-2301335-1.patch30.77 KB

Updated to work with Drupal 8.0.0-beta4. Same outstanding problems as previous patch.

Will try to put this up on a code-hosting website when time permits so it easier to try.

lahoosascoots’s picture

lahoosascoots commented

Hey! Looks like this has been pretty dormant. Mind if I take a swipe at getting everything up to the latest beta?

I'd put it up in a repo in https://github.com/d8-contrib-modules where a team of us have been working to port a ton of modules. Once we get a working module we'd move everything back here to get it released.

lahoosascoots’s picture

lahoosascoots commented

I've got this in a good state in https://github.com/d8-contrib-modules/browscap

I've converted the getBrowser into a service to better adhere to best practices.

It's got some testing for the import using a mock object to use a local ini file. I've also sorted out the performance issues and got the install down under 60 seconds. The performance upgrades came from https://www.drupal.org/node/2418473

A review of the code would be appreciated and PRs are of course welcome.

lahoosascoots’s picture

lahoosascoots commented

I've got this in a good state in https://github.com/d8-contrib-modules/browscap

I've converted the getBrowser into a service to better adhere to best practices.

It's got some testing for the import using a mock object to use a local ini file. I've also sorted out the performance issues and got the install down under 60 seconds. The performance upgrades came from https://www.drupal.org/node/2418473

A review of the code would be appreciated and PRs are of course welcome.

aohrvetpv’s picture

aohrvetpv commented

Very nice. I will try to try it out soon. The original port I posted in this issue was severely out of date and only partially completed.

lahoosascoots’s picture

lahoosascoots commented

IMO, the module is ready for a RC release. Think you can have a look and take care of this or potentially add me as a co-maintainer to help facilitate the release process?

aohrvetpv’s picture

aohrvetpv commented

Sorry to have not tested it yet. Have to upgrade my development environment to get the minimum PHP now needed for D8.

I think per the release naming conventions it should probably start out as alpha not RC, since it hasn't been much reviewed and in case of major bugs.

I am only maintainer to maintain the D6 branch so I probably shouldn't add maintainers, if I even can. But other maintainers may be willing to add you.

aohrvetpv’s picture

aohrvetpv commented

For what it's worth, I have my development environment upgraded now, and plan to try the port out later today.

aohrvetpv’s picture

aohrvetpv commented

For consideration for Drupal 8 port: #2573711: Switch URLs to HTTPS where possible

aohrvetpv’s picture

aohrvetpv commented

Two minor UI bugs:

1. After pressing "Save Configuration":

    Password Strength settings have been stored
    The configuration options have been saved.

First message does not make sense.

2. Closing parenthesis needed for "(Requires a correctly configured cron maintenance task."

aohrvetpv’s picture

aohrvetpv commented

Looks and seems to work great. Thanks for making this port.

I did some simple manual testing of the administration UI and the getBrowser() method.

Have not fully read the code, but I did skim through it. Some minor comment or coding standards issues. But at least to me it seems commit worthy. Should it be 8.x-2.x since the functionality provided is about the same as 7.x-2.x and 6.x-2.x? (If I recall correctly, the obsolete 1.x branches allowed for tabular display of logged user agents or browser information, but that feature was dropped in 2.x.)

No Drush commands as in D7 but those seem nonessential.

greggles is probably the maintainer to get this committed or grant maintainership.

Also, probably improper to set this issue RTBC without a patch.

aohrvetpv’s picture

aohrvetpv commented

While I remember: Might be worth commenting why varchar size is 191. My guess, which from wc -L looks to be correct, was it is to fit the largest user agent pattern in the current Browscap data. AFAIK, we have no guarantee they won't exceed 191 (or even 255) later. Maybe to be safe we should sometime add code to warn and skip the pattern in the case that it exceeds the maximum.

lahoosascoots’s picture

lahoosascoots commented

I'll get your recommendations worked in. Some look like relics of the code I snatched the configFormBase from. If you find anything else I'd prefer if you file an issue against the GitHub repo until we release on d.o.

"Should it be 8.x-2.x since the functionality provided is about the same as 7.x-2.x and 6.x-2.x?"

Likely. However it does now provide this function via a service, so it may be 8.x-3.x to note the change in how the functionality is used. That would be a talking point I'd like input on.

"Might be worth commenting why varchar size is 191"

It was throwing errors on install at 255. Don't quite remember exactly what that error said. I'm going to file an issue against 8.x once we get it up on d.o to see if anyone else has any insight on it.

"Also, probably improper to set this issue RTBC without a patch."

That's my final step once I get some eyes on it for a preliminarily review. I'll be posting it in this issue.

lahoosascoots’s picture

lahoosascoots commented

exception 'PDOException' with message 'SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes'

is the error thrown when using a 255 byte length on the useragent.

lahoosascoots’s picture

lahoosascoots commented

Also fixed all of your UI recommendations and converted links to HTTPS

aohrvetpv’s picture

aohrvetpv commented

"Should it be 8.x-2.x since the functionality provided is about the same as 7.x-2.x and 6.x-2.x?"

Likely. However it does now provide this function via a service, so it may be 8.x-3.x to note the change in how the functionality is used. That would be a talking point I'd like input on.

IMHO, either 8.x-2.x or 8.x-3.x would be fine. 8.x-1.x would seem wrong since this port does not have the same features as 7.x-1.x/6.x-1.x. Maybe others would have an opinion on 8.x-2.x vs. 8.x-3.x.

From https://www.drupal.org/node/156119:

Optionally, project maintainers could create additional branches, which would have higher major versions (2, 3, etc.). These additional major revisions will be for whatever the maintainer of the project sees fit. It is up to the project maintainer to document on their project node and in their releases how they're using their own branches and what users of their modules should expect from any available major revision numbers. By convention, if the maintainer is adding major new features to a module or doing a complete rewrite, they would use major revision 2 or higher.

lahoosascoots’s picture

lahoosascoots commented
Assigned: Unassigned » lahoosascoots
StatusFileSize
new 2301335-port-browscap-to-d8-17.patch66.63 KB

Port patch attached. Contains anything added by me since 31cfe4ef839c4be79ba578227d743214f5414fc7

lahoosascoots’s picture

lahoosascoots commented
Version: 8.x-1.x-dev » 8.x-3.x-dev

Moving to correct major version

lahoosascoots’s picture

lahoosascoots commented

This is posted to the 8.x-3.x-dev branch. Once it's RTBC I'll post an alpha.

lahoosascoots’s picture

lahoosascoots commented
Status: Needs review » Fixed

Been a year with no objections, I'm posting an alpha and closign this out. XD

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.