hook_views_pre_view incorrect documentation example

Thanks for the issue! Marking Novice and fixing the issue summary slightly. Also this should be fixed in 8.2 and then it should be applied to the other 8.x versions (probably the same patch will apply to all).

Hi,

I have created a patch to address the request, but I have also found other instances of wrong use for $view->name.

There is one in hook_views_post_build and another in hook_views_pre_execute.

Please review.

Thanks.

PS: Patch also works for 8.0.x and 8.1.x.

Looks good! Thanks for fixing the other instances of $view->name in the same file. That is a perfectly excellent expansion of the issue scope.

I grep in the entire code base and found no other instances except in this file. Please let me know should you find more.

Hi @jhodgdon,

I have found in https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Views.p... the following documentaton:

views object (containing $exclude_view->storage->name and $exclude_view->current_display)

Does this case apply as well? Looked around in the codes and can see that in the following for example:
https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Plugin%...

Line 162 uses: if ($view_id && $this->view->storage->id() != $view_id) {

I am not particularly sure about this one so I need anyone to check this.

I can't make my mind up about the scope of this issue - whether or not to fix all the things in views.api.php that are broken... for example hook_field_views_data_views_data_alter has $entity_type_id = $field->entity_type; which should be $entity_type_id = $field->entityTypeId(); and I think $table_mapping = \Drupal::entityManager()->getStorage($entity_type_id)->getTableMapping(); is wrong too. In other places $view->query->tables - the tables property does not exist as far as I can see. Same for $view->query->table_queue. And hook_views_query_alter also needs work.

Well that is a good question, and probably not a novice issue if the scope is "Fix all the outdated stuff in views.api.php". Your call... I have been getting the scope wrong on issues lately and am not making this decision.

Some of these fixes have already been changed in previous commits it seems. I can't speak to the scope of this issue, but I re-rolled #6 to get this patch ready in it's current state for 8.6.x.

I tested the patch, and the patch worked correctly.

i also reviewed the patch and i saw that they were using the old coding standards for array and replaced for the new standard.

Thanks for the clarification on that. I saw the array was done differently between versions and I was unsure about that change.

This is a weird case because the original issue is already fixed in 8.6.x. The latest patch fixes something different. I don't see anything wrong with it, but just wanted to point that out.

Triggering a 9.5 build. Once that's complete I'll RTBC

Looks good.

The issue summary needs to be updated; it doesn't describe what the patch is effectively fixing.

(I am changing two issue tags, since this patch won't be back-ported to branches that are now unsupported.)

@apaderno updated the summary if that helped.

The documentation for hook_views_pre_view() doesn't contain any reference to $name nor name(). That is not even what the patch is changing, since it changes the documentation comment for the Views::getViewsAsOptions() method, which is using the following documentation comment.

  /**
   * Returns an array of view as options array, that can be used by select,
   * checkboxes and radios as #options.
   *
   * @param bool $views_only
   *   If TRUE, only return views, not displays.
   * @param string $filter
   *   Filters the views on status. Can either be 'all' (default), 'enabled' or
   *   'disabled'
   * @param mixed $exclude_view
   *   View or current display to exclude.
   *   Either a:
   *   - views object (containing $exclude_view->storage->name and $exclude_view->current_display)
   *   - views name as string:  e.g. my_view
   *   - views name and display id (separated by ':'): e.g. my_view:default
   * @param bool $optgroup
   *   If TRUE, returns an array with optgroups for each view (will be ignored for
   *   $views_only = TRUE). Can be used by select
   * @param bool $sort
   *   If TRUE, the list of views is sorted ascending.
   *
   * @return array
   *   An associative array for use in select.
   *   - key: view name and display id separated by ':', or the view name only.
   */
  public static function getViewsAsOptions($views_only = FALSE, $filter = 'all', $exclude_view = NULL, $optgroup = FALSE, $sort = FALSE) {
    // …
  }

The last attached patch contains the following lines.

--- a/core/modules/views/src/Views.php
+++ b/core/modules/views/src/Views.php
@@ -285,7 +285,7 @@ public static function getDisabledViews() {
    * @param mixed $exclude_view
    *   View or current display to exclude.
    *   Either a:
-   *   - views object (containing $exclude_view->storage->name and $exclude_view->current_display)
+   *   - views object (containing $exclude_view->storage->id() and $exclude_view->current_display)
    *   - views name as string:  e.g. my_view
    *   - views name and display id (separated by ':'): e.g. my_view:default
    * @param bool $optgroup

The issue summary and the issue title must be updated.

$exclude_view->storage->id() seems to still be not correct, since neither the Views class nor the DisplayPluginBase class seems to have a $storage property.

Furthermore, if the purpose of this issue is fixing the documentation comment for Views::getViewsAsOptions(), there are other fixes to apply.
For example, instead of views object, it should use Views object; it should not use colons followed by e.g. but a comma followed by for example; it should not use display id but display ID.

Hello, i worked in this issue, and did the changes asked by @apaderno on #33.
And about the comment #32 from @apaderno too, talking about the $exclude_view->storage->id(), i think that this way as are, its right because storage is a service from core, and is used in the same file at line line 325.

And sorry @pradhumanjainOSL, but i don't know if you know, but usually in core issues, we should don't assigne to ourself, only send the pathc, and made comments.
this link below explain better about that.
https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

To help compare what was changed can you provide an interdiff please

Applied patch in #35 successfully on 9.5x-dev. The patch has done the changes mentioned in #33 and changed '$view->name' to '$view->id()'.

@WagnerMelo I m new to contribution. Thanks for giving me the information about not assigning the core issues.

Sorry about that, here are the interdiff @smustgrave.

NP @pradhumanjainOSL we are here to learn together.

Hi, Thanx @WagnerMelo for the patch, As you corrected the documentation as per the comment #32 and #33, it looks great and relatable, I have tried to applied it successfully on 10.1.x it applied cleany, so it can be compatible for 10.1.x.

Moving to RTDC for now till further update.

+++ b/core/modules/views/src/Views.php
@@ -287,9 +287,9 @@ public static function getDisabledViews() {
+   *   - views name as string,for example my_view
+   *   - views name and display ID (separated by ':'), for example my_view:default

I think the examples should be in single quotes and if we're changing these lines we should break the lines on 80 chars.

I would expect the docblock to end up looking something like:

   * @param \Drupal\views\ViewExecutable|string $exclude_view
   *   View or current display to exclude.
   *   Either a:
   *   - View executable object
   *   - View name, for example: 'my_view'
   *   - View name and display ID separated by ':', for example: 'my_view:page'

I have uploaded the patch
Addressed #43 point.

@Anchal_gupta how about fixing the 80 chars issue and the @param types too - as per #43.

Try to address comment #43.

Just a small change: The first line is missing a space after the comma and before for example..

+   *   - views name,for example 'my_view'
+   *   - views name and display ID separated by ':', for example 'my_view:page'

Try to address #48.

The patch fixes also the line longer than 80 characters. This is RTBTC.

Backported to 9.4.x as this is a docs bug fix.

Committed and pushed d88758034c to 10.1.x and 4467280292 to 10.0.x and 9ebe2b579b to 9.5.x and 6f71312b3f to 9.4.x. Thanks!