Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

Issue summary: View changes
Parent issue: » #2044421: [meta] Upgrade to Drupal 8
drunken monkey’s picture

Also possibly worth a thought: introduce a proper method for checking for data type support in a service class, instead of using a "magic name" supportsFeature() call.

mollux’s picture

Assigned: Unassigned » mollux
mollux’s picture

Status: Active » Needs review
FileSize
13.3 KB

The attached patch provides the datatype plugin functionality and cleans up the getdatatype() logic to be more useful.
Things to do:
- write tests
- add documentation page with all the datatypes.

drunken monkey’s picture

Status: Needs review » Needs work

Great work, Mattias, thanks!
I didn't try it out yet, but I looked over the code and saw only a few minor issues:

  1. +++ b/search_api.services.yml
    @@ -18,3 +18,7 @@ services:
         arguments: ['@database', '@entity.manager']
    +
    +  plugin.manager.search_api.datatype:
    +    class: Drupal\search_api\Datatype\DatatypePluginManager
    +    parent: default_plugin_manager
    

    Unless I'm mistaken, services should generally be sorted alphabetically, so this should go further up.

  2. +++ b/src/Datatype/DatatypeInterface.php
    @@ -0,0 +1,43 @@
    + * Contains \Drupal\search_api\Datatype\DatatypeInterface.
    

    In my eyes, this looks weird, I'd prefer DataType for both namespace and class names. It's the same in method names already, after all.
    Of course, we use "datasource" as one word, but I think that's a special case.

  3. +++ b/src/Datatype/DatatypeInterface.php
    @@ -0,0 +1,43 @@
    +  /**
    +   * Converts a value .
    +   *
    +   * @param mixed $value
    +   *   The value to convert.
    +   *
    +   * @return mixed
    +   *   The converted value.
    +   */
    +  public function getValue($value);
    

    Incomplete doc comment.

  4. +++ b/src/Datatype/DatatypeInterface.php
    @@ -0,0 +1,43 @@
    +  /**
    +   * Returns the fallback default datatype for this datatype.
    +   *
    +   * @see \Drupal\search_api\Utility::getDefaultDataTypes()
    +   *
    +   * @return string
    +   *   The fallback default datatype.
    +   */
    

    @see should come after @return. See the documentation standards.

  5. +++ b/src/Datatype/DatatypePluginBase.php
    @@ -0,0 +1,53 @@
    +namespace Drupal\search_api\Datatype;
    +use Drupal\search_api\Plugin\IndexPluginBase;
    

    These should be separated by an empty line.

  6. +++ b/src/Datatype/DatatypePluginBase.php
    @@ -0,0 +1,53 @@
    +}
    \ No newline at end of file
    

    Missing trailing newline.

  7. +++ b/src/Utility.php
    @@ -80,46 +77,97 @@ class Utility {
    +   *   A nested associative array with the default types as keys, mapped to their
    

    Overlong comment line.

  8. +++ b/src/Utility.php
    @@ -80,46 +77,97 @@ class Utility {
    +        'label' => \Drupal::translation()->translate('Fulltext'),
    

    I think just using the t() function is fine here, too. You should only use the OOP way if you can inject the translation manager.

  9. +++ b/src/Utility.php
    @@ -80,46 +77,97 @@ class Utility {
    +   * @return string[]|null
    +   *   If $type was not given, an array containing all data types, in the
    +   *   .
    

    Incomplete doc comment.

mollux’s picture

Issue tags: +drupaldevdays
mollux’s picture

FileSize
29.39 KB

Fixed the issues above + wrote the basic tests to check the functionality. UI tests should be added.

mollux’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2470813-custom-data-types_plugins-7.patch, failed testing.

mollux’s picture

Aaargh, the tests moved. Re-roll will be for tomorrow

mollux’s picture

Status: Needs work » Needs review
FileSize
29.47 KB

re-roll

Status: Needs review » Needs work

The last submitted patch, 11: 2470813-custom-data-types_plugins-11.patch, failed testing.

mollux’s picture

Status: Needs work » Needs review
FileSize
29.54 KB

fixed the tests

mollux’s picture

FileSize
31.65 KB

Added webtest to check that the custom data type can be selected. This is normally RTBC :)

The UI stuff (overview page of (custom) data types ant their description + making it visible if the data type of a field isn't supported by the backend) will be tackled in a separate issue.

drunken monkey’s picture

Thanks, great work, looks very good already!
Just had a few code style fixes, in the attached patch. Please reviw to see if you agree with them. (The empty line before the end of a class is my personal taste, but currently (I think) uniform within the module.)
Also, it seems this is still missing the page listing all data types? Or is there already an issue for that?

Status: Needs review » Needs work

The last submitted patch, 15: 2080791-15--data_type_plugins.patch, failed testing.

mollux’s picture

Status: Needs work » Needs review
FileSize
31.39 KB
663 bytes

As discussed, I changed the test code to use the magic method again. For me the other changes are ok!

drunken monkey’s picture

Status: Needs review » Fixed

Great, committed.
Thanks again!

  • drunken monkey committed a9aa690 on 8.x-1.x authored by mollux
    Issue #2080791 by mollux, drunken monkey: Added support for custom data...

Status: Fixed » Closed (fixed)

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