Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Provide an interface for the random string generator #3

Closed
wants to merge 2 commits into from

Conversation

pfrenssen
Copy link
Collaborator

The random string generator \Drupal\Component\Utility\Random is currently hardcoded in the project. It would be better if this conformed to an interface so it can be properly injected. This would allow us to override it, and it allow the methods to be autocompleted by IDEs.

@jhedstrom
Copy link
Owner

Sorry for the delayed response.

The reason there isn't an interface here already is that the random generator has been plucked directly from Drupal 8 (I was hoping the components there would eventually be available individually via composer.json).

Ideally, I'd like to see Drupal 8 adopt an interface approach, but in the meantime, I think this approach makes sense to do here.

Somebody on twitter mentioned using https://github.com/rchouinard/rych-random as a random generator, I wonder if that would be possible still with this approach?

@pfrenssen
Copy link
Collaborator Author

I have used rych-random in a previous project but we eventually abandoned it for a self rolled solution since it is rather basic. What is very nice about it is that it scans your system for available random number generators and will use the best available.

We decided to abandon it because in testing it is very interesting to be able to retrieve valid data as well as invalid data, and also to be able to extend it so you can test things like URLs, email addresses etc.

This is the interface we used, it served us pretty well (ref http://cgit.drupalcode.org/paddle_selenium_tests/tree/testdataprovider/Kanooh/TestDataProvider/TestDataProviderInterface.php?h=7.x-1.x):

<?php


/**
 * @file
 * Contains \Kanooh\TestDataProvider\TestDataProviderInterface.
 */

namespace Kanooh\TestDataProvider;

/**
 * Interface for TestDataProvider classes.
 */
interface TestDataProviderInterface
{
    /**
     * Returns an array of valid test data.
     *
     * @return array
     *   An array of valid test data.
     */
    public function getValidDataSet();

    /**
     * Returns an array of invalid test data.
     *
     * @return array
     *   An array of invalid test data.
     */
    public function getInvalidDataSet();

    /**
     * Returns a random value from the valid data set.
     *
     * @return mixed
     *   A valid value.
     */
    public function getValidValue();

    /**
     * Returns a random value from the invalid data set.
     *
     * @return mixed
     *   An invalid value.
     */
    public function getInvalidValue();
}

This can be coupled with a base class that implements getValidValue() and getInvalidValue().

Here is the full set of classes: http://cgit.drupalcode.org/paddle_selenium_tests/tree/testdataprovider/Kanooh/TestDataProvider?h=7.x-1.x

@@ -82,6 +83,7 @@ public function __construct($alias = NULL, $root_path = NULL, $binary = 'drush',

$this->binary = $binary;

// @todo This should not be optional but injected by a factory method.
if (!isset($random)) {
$random = new Random();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't actually work as written now, since Random won't be available without a use statement. I'd be happy to change this to using a factory method if you want to take a go at that.

@jhedstrom
Copy link
Owner

I'd love to actually get an interface into Drupal core instead of just the Random class.

@jhedstrom
Copy link
Owner

If my feedback above in the DrushDriver is resolved, I'm open to making this change as-is, and we can always follow-up in core.

@jhedstrom jhedstrom added this to the 1.2.0 release milestone Sep 2, 2015
@jhedstrom jhedstrom modified the milestones: 2.0, 1.2.0 release Jun 16, 2016
@jhedstrom
Copy link
Owner

We no longer directly hardcode the Random class, but rather require it via composer. I think the proper fix is to get an interface into core.

@jhedstrom
Copy link
Owner

I've opened https://www.drupal.org/project/drupal/issues/2969060. Closing this out for now.

@jhedstrom jhedstrom closed this May 2, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants