The serialization fails if used with Views REST export

Created on 24 May 2023, over 1 year ago

Problem/Motivation

If the $first_row in Xls::extractHeaders is a single string, a PHP error is generated.

The website encountered an unexpected error. Please try again later.
TypeError: array_keys(): Argument #1 ($array) must be of type array, string given in array_keys() (line 313 of modules/contrib/xls_serialization/src/Encoder/Xls.php).

This can happen if the user isn't authenticated to a View REST export that uses this serialization.

See the core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php function on4xx that executes the serialize command

$format = $request->getRequestFormat();
$content = ['message' => $exception->getMessage()];
$encoded_content = $this->serializer->serialize($content, $format);

Steps to reproduce

  1. Create a REST export View with access defined so that you have user that cannot access the REST export.
  2. Configure generally the REST export to work if user is authenticated. Set URL for the REST export.
  3. Now with a user that doesn't have access to that new REST export, try to access it directly with URL of the REST export.
  4. PHP error occurred.

Proposed resolution

Support cases when the content contains error message.

Remaining tasks

Write patch to ensure serialization doesn't fail if user is unauthenticated to use views REST export.

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Active

Version

1.3

Component

Code

Created by

🇫🇮Finland MikaT

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @MikaT
  • 🇪🇸Spain ajimenezz

    Hello, is there some solution to this?

    I have the same error here:

    The website encountered an unexpected error. Please try again later.
    TypeError: array_keys(): Argument #1 ($array) must be of type array, string given in array_keys() (line 313 of modules/xls_serialization/src/Encoder/Xls.php).
    array_keys() (Line: 313)
    Drupal\xls_serialization\Encoder\Xls->extractHeaders() (Line: 169)
    Drupal\xls_serialization\Encoder\Xls->setHeaders() (Line: 75)
    Drupal\xls_serialization\Encoder\Xls->encode() (Line: 40)
    Symfony\Component\Serializer\Encoder\ChainEncoder->encode() (Line: 306)
    Symfony\Component\Serializer\Serializer->encode() (Line: 129)
    Symfony\Component\Serializer\Serializer->serialize() (Line: 77)
    Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber->on4xx() (Line: 112)
    Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase->onException()
    call_user_func() (Line: 142)
    Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (Line: 219)
    Symfony\Component\HttpKernel\HttpKernel->handleThrowable() (Line: 91)
    Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 57)
    Drupal\Core\StackMiddleware\Session->handle() (Line: 47)
    Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
    Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
    Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 47)
    Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 52)
    Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 23)
    Stack\StackedHttpKernel->handle() (Line: 717)
    Drupal\Core\DrupalKernel->handle() (Line: 19)

  • 🇨🇦Canada danrod Ottawa

    I am having the same issue here, I'll try to submit a patch today.

  • First commit to issue fork.
  • Pipeline finished with Failed
    2 months ago
    Total: 136s
    #313051
  • 🇳🇿New Zealand ericgsmith

    This issue is not only applicable to the rest export - it is also applicable to the excel export and views data export modules when authentication is in place.

    I have fixed this with 2 changes

    I have made changes so the encoder more flexible for the data it is provided with. At the start of encode the encoder is casting everything to an array, but from that point it assumes that it has a nested array. It also makes an assumption that the row keys are numeric - but they do not have to be. So changes have been make to fix the type errors.

    Once the issues in encode were fixed the endpoint was still returning errors:

    Exception: Deprecated function: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated
    Drupal\Core\EventSubscriber\ActiveLinkResponseFilter->onResponse()() (Line: 85)

    This is due to this module using an event subscriber to add the format. This was one common, but now the recommend approach is to use a service provider to register the formats as middleware. CSV serialization implemented the same change 🐛 PHP 8.1 deprecated function warning Fixed which is based on how several other serializers are doing this, e.g:

    - https://git.drupalcode.org/project/hal/-/blob/2.x/src/HalServiceProvider...
    - https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/jsona...
    - https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...
    - https://git.drupalcode.org/project/csv_serialization/-/blob/4.x/src/CsvS...

  • Pipeline finished with Success
    2 months ago
    Total: 142s
    #313055
  • 🇳🇿New Zealand ericgsmith

    Setting to needs review.

    Please note the pipeline is failing as the gitlab ci config is to test the current major - which is Drupal 11 - but this module is not Drupal 11 compatabile.

    I run the pipeline with the variable set to test on the previous major (i.e test on Drupal 10) and we can see the test passing here https://git.drupalcode.org/issue/xls_serialization-3362321/-/pipelines/3...

  • Pipeline finished with Failed
    2 months ago
    Total: 136s
    #313062
  • 🇳🇿New Zealand RoSk0 Wellington

    There is one line that has to go, otherwise this is RTBC.

    Thank you

  • 🇳🇿New Zealand ericgsmith

    Thanks @rosk0 - removed the duplicate comment - thank you for the reviews.

  • Pipeline finished with Failed
    2 months ago
    #318649
  • 🇫🇷France mably

    @ericgsmith this also looks like a good candidate for the 2.0.x branch.

  • Pipeline finished with Success
    28 days ago
    Total: 181s
    #348789
  • Pipeline finished with Success
    28 days ago
    Total: 148s
    #348790
  • Pipeline finished with Success
    28 days ago
    Total: 329s
    #348793
  • Pipeline finished with Success
    28 days ago
    Total: 273s
    #348796
  • 🇳🇿New Zealand ericgsmith

    Rebased MR + opened a new MR into 2.0.x branch.

    For me this is a bug fix - so can go into either - but depending on your approach may need BC handling. I'll note that Drupal cores BC handling for event subscribers says:

    Class implementations of paramconverters, access checkers, event subscribers and similar services which are never expected to be used directly either as services or value objects, are not considered part of the API. You should not extend from these classes and provide your own implementation instead. (However, note that service machine names are public API and changing them requires a deprecation process.)

    So I guess technically you could keep the service definition in place with an empty subscriber. Not sure what that would achieve really - also 2.0.0 is released so if the argument here is that its a BC break then you will need 3.x - I think that would be overkill for an internal class.

  • 🇫🇷France mably

    Hi @ericgsmith, previous BC breaking changed didn't seem to annoy anyone, and 2.0.0 has been just released.

    Let's just add this to the 2.1.x branch I just created.

  • 🇳🇿New Zealand ericgsmith

    We probably also do need an empty hook_post_update_NAME to force a cache clear

Production build 0.71.5 2024