πŸ‡³πŸ‡±Netherlands @Lendude

Amsterdam
Account created on 9 January 2010, about 14 years ago
  • iOΒ  …
#

Merge Requests

More

Recent comments

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Sounds a little edge case to me, don't think it makes the 80% criterium, especially with token support thrown in. Might be more user friendly with a select element showing available languages

It would need some things I think:

  • should only be shown on multi-lingual sites
  • yeah validating if we have valid input sounds good, just hoping it falls back sounds bad
  • tests
  • a schema update
  • an upgrade path for the schema change
  • a test for the upgrade path
πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Please search the Views and Views UI modules (and maybe others?) for 'Link display' and update all occurrences everywhere. Please make sure the changed sentences still make sense, not sure a find/replace is going to work everywhere

Did anybody ever look into moving this out of the pager section? I agree with @dawehner in #1 that this feel like a weird place for this.

Do we need to think about updating the machine name of the setting too? We now have a big difference between the name of the field in code and the name in the UI which can lead to more confusion but now from a DX perspective

Do we need to run the new label past some UI experts and make sure we all agree this is the best we can come up with, 'Destination' feels a bit vague to me.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Thanks for opening this, it reads like a duplicate of πŸ› Ellipsis in pager template fails accessibility tests Needs work so closing for new, feel free to reopen this if you feel this is addressing a different issue.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Well if there is a use for it, we can always undeprecate it (or in this case, never deprecate it in the first place), or if it needs a string version of this we should add a string version and put it in the pgsql module where this seems to belong (is PostgreSQL the exception here or is MySQL? How does SQLite handle this?)

Also, I think the general consensus is that int joins are more performant than string joins, so what performance impact does casting everything to a string have? We should check.

How is this handled outside Views? It's not like Views is the only place you can make joins. Is there maybe some method we could/should add to the database drivers that handles this?

I would like to see test coverage added that actually shows we need this, so something that would fail without this fix. All the modified test coverage changes only show that the ':text' is added and this doesn't break existing queries.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Did some clean up, no idea what spellcheck is complaining about.

I think we still need an upgrade path test for this

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Rereading this, feels like a duplicate of πŸ› Switching on aggregation generates fatal "Column not found: 1054 Unknown column" SQL error when using multi-column Fields Needs work can somebody check if the work there helps fix their issues?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Nice find, fix looks good, but we should probably add a test for this, shouldn't be too hard to add something to \Drupal\Tests\views\FunctionalJavascript\ClickSortingAJAXTest I think?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Tried to reproduce:
* Clean Umami install
* Enabled Comment module
* Checked key_value table, comment entry was there
* Uninstall comment module
* Checked key_value table, comment entry was gone

Can you reproduce this on a clean Drupal install?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Added some more modules

This test printed output: int(37)

Checked all modules that implement hook_views_data, think this get all of the ones that don't need extra handling or just reuse existing plugins...I think

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Forgot to untag

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

The "Views Default Argument: Entity Field Values" module already provides this functionality and much much more, see https://www.drupal.org/project/views_arg_entity_field β†’

I don't think we should be adding something so Node specific to core. The problem with these default arguments is that they are not filtered and all of them always show up on all fields even when totally inappropriate (filtering a date field by current node type when viewing a taxonomy term anyone?), so I'd prefer the list we have by default in core to stay as small as possible. Yes default arguments are great tools but having a generic solution like the module provides is, I think, a better way forward than adding individual fields to core.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

This checkbox comes from \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceLabelFormatter, Views just reuses the field formatter, so not sure if this is something Views should be handling or the FieldFormatter itself.

@larowlan is πŸ“Œ Fall back to 'edit-form' as default $rel in EntityBase::toUrl() Fixed the change you are referring to? The formatter seems to catch the new exception thrown there so not sure.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

@daffie the visual representation leaves a little to be desired to make it clear, but if you look at the top level Postgres build you can see that the normal test runs pass and only the test-only run fails, as we would hope

https://git.drupalcode.org/issue/drupal-3415118/-/pipelines/79102

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

PostgreSQL test-only fails as expected: https://git.drupalcode.org/issue/drupal-3415118/-/jobs/653536

So this looks good to me

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Nice, moved to a MR and lets see if the test run better there, works fine locally

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Nice find, we need a test for this, I've already written one in πŸ› A number of Views combined fields filter operators are currently pointless to an end user Needs work that just hasn't landed yet.

Probably easiest to grab the REGEX test from that and add it here and see if it indeed fails on PostGreSQL

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Can we use \Drupal\views\Plugin\views\join\CastedIntFieldJoin for this and avoid adding pgsql specific code to base plugins?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Moved code to a MR to provide some feedback

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

This seems to have been fixed in #2899248: Don't reimplement validation rules for workflow state add/edit forms in ::copyFormValuesToEntity β†’ where this was added

if (!$form_state->isValidationComplete()) {
      // Only do something once form validation is complete.
      return;
}

Am I reading that right or is there still something broken here?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

#3 and #9 outline nicely why we can't just use the field's labels, because the same field can have multiple sets of labels. So the goal set out in the title I don't think we can do.
The option set out in #9 where we provide fields in the boolean filter where you can configure this could work, but does that really offer much more value over just using a grouped filter for this? I don't think it does, so "Won't fix" for me, unless we can think of way to determine the values we should be using here

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Did a check:
Investigation: See for example https://www.drupal.org/project/drupal/issues/2404603#comment-9576291 β†’ for the test in an earlier form, where the two parameters made sense (they contained different things)
Conclusion: leftover code from previous iterations of the test that never got committed in that form and was missed in review

Added to the IS

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Custom Date field

What does that mean? Is it a date field added through the UI or a base field or some type of custom date field you've build?

Per #4, please add some steps to reproduce this on a clean Drupal install, that would really help

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Check all your Views that have a taxonomy term filter and try to resave them manually. Does that work? Are there any broken handlers on them? After resaving manually, does the update run?
All the update does if resave the View if it has such a taxonomy term filter, and apparently that is failing, usually this is due to a pre-existing problem in the View and has very little to do with the actual update. The update just surfaces the issue.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

We actually have a Views test that uses a batching action: \Drupal\Tests\views\Functional\UserBatchActionTest::testUserAction

Not sure if this issue can be replicated with just core, but if it can, it would be good to add a test at some point.
I agree that we don't need to add coverage in this issue, fixing the regression is more important, but I would suggest making a follow up issue to expand the coverage in that test a bit, it's very minimal as it stands.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

We really need some steps to reproduce this if we are going to keep this open. Is this still relevant? If so, when?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Can we track down what changed here in 10.2? Neither files touched by the current fix were changed recently, it would be nice to track down what caused this so we can maybe prevent other side effect and make sure we are on the right track with fixing this here and not just band-aiding a much bigger issue

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

I tried to rebase some stuff into 11.x but no luck Β―\_(ツ)_/Β―

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Lendude β†’ changed the visibility of the branch 2829178-11.x to hidden.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Lendude β†’ changed the visibility of the branch 10.1.x to hidden.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

The current fix changes way more than just the row plugins, as pointed out in the IS, this also affects things like pagers. This isn't necessarily a bad thing but we do need to check if we are not hiding things we don't want hidden and maybe expand the test coverage to also cover other plugin types.

For #39, I don't think plugins that have display_types = {"normal"} set need to change anything, it just becomes redundant, because we are adding display_types = {"normal"} for all plugins that don't set anything. I think....

But yeah changing this in ViewsPluginManager feels a little bit more invasive than it did when this was opened, I feel we do need a clearer list of all affected plugin types if we do it in ViewsPluginManager

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Checked and indeed, as @longwave pointed out in #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator β†’ , $link_html is never used other than for setting $final_html, so we might as well just use $final_html always.

Not a bug, just a bit of cleanup really.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Fixed the tests, did some manual testing, updated the IS

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Moved to MR, lets see what tests fail now and fix them

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

I'm not sure the name is even available in the database for the sake of sorting

It is not, it's config after all. So moving to a feature request. Would be nice to be able to do......

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Checking old issues for the Bug Smash initiative.

Tested this, and indeed, with the new email permission this seems to work, without you get a 404 is you try to show the email.

If there was another issue here, please feel free to reopen this

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug smash Initiative.

Yes, this is messy. The underlying problem is #2917814: Views table style info never gets updated when changing/adding fields β†’ so going to close this as a duplicate of that. If you feel this is talking about a different issue, please feel free to reopen this.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the bug smash Initiative

Tried to reproduce this and can't anymore. This seems to have gotten fixed. If this is still a problem for anybody, please feel free to reopen this issue

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Looking at this again, this is a "works as designed" for me. The combined field filter is not meant to work for all possible use cases. More complex search questions should be handled by using something like Search API and Facets.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative

I still see this happening, but it's not blocking anything as far as I can tell, only throwing some errors on the Preview. Once you save the View, everything is fine (until you switch between types again).

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old bugs for the Bug Smash Initiative.

#4 Sounds like this might have been solved by other issues.

If this is still an issue please feel free to reopen this

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative

I would expect you to always get just 50 results, so the 200 sound strange. Should this still be a problem, please provide some steps to reproduce your issue.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative.

πŸ› Database reserved keywords need to be quoted as per the ANSI standard Fixed will have fixed this hopefully, if this is still an issue for somebody please feel free to reopen this.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative.

I'd say this is by design, the field is hidden, which in Views lingo means it shouldn't be rendered, so it's not passed on to the rendering layer.

If you feel that this is something that is needed we could reopen this as a feature request, but not sure this would fit the 80% case.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative.

Now you get a fairly clear message: "No valid values found on filter: Content: Something." on the Views UI and the View stop displaying results until you fix it.

We could add a config dependency to this field, but as always, the problem then becomes, what is the correct action to take? Just stop filtering on that value? That might lead to a information disclosure. If this was the only value filtered on, it's not doing anything, should we remove it?

So, I think, what happens here is right, make the user fix the View and don't show anything until they do.

Other thoughts?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative.

Search API seems to have found a way to work without this, so currently I don't think we should change this until we have a clear idea of what/if it is blocking.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative.

Some work was done for checkboxes here #2651102: Using checkboxes for exposed filters results in zero rows displaying β†’ also still some work to be done for dates in πŸ› Views exposed group filter (with option allow multiple selections) generates notices when using date and date range fields Needs work but not getting this error anymore so that part seems to have been fixed

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old bugs for the Bug Smash Initiative

The rearrange works now, as far as I can tell, the styling isn't great, but if you switch off javascript in Claro and then view the Views UI, this is not the first thing you trip over by far.

So if we start caring about the layout of nojs, I think that should be a whole new thing.

Closing this for now

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

It's a deprecation message.

@trigger_error('The extension.list.module service must be passed to ' . __NAMESPACE__ . '\SystemController::__construct. It was added in Drupal 8.9.0 and will be required before Drupal 10.0.0.', E_USER_DEPRECATED);

Not passing extension.list.module to the constructor is deprecated, so like the message says, before to mark your module ready for D10, you need to pass this to the constructor if you are extending SystemController, or it will break....which it did

So that module probably extends SystemController (I didn't check) and they need to update their code, so let's move the issue to that queue

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Going through old issues for the Bug Smash Initiative.

This still happens, closing this as a duplicate of πŸ› Views doesn't parse twig when there are no tokens to replace Needs work . Technically that issue should be a duplicate of this, but more activity there so lets close this one.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Tried to reproduce this on a clean Umami install but not problems there as far as I can tell

Steps done:
* Add a new Vocabulary
* Add permissions for this Vocabulary to the Editor and Author roles
* Check the config for these roles in config sync (/admin/config/development/configuration/single/export) and make sure the permissions are there
* Delete the Vocabulary
* Check the config for these roles in config sync (/admin/config/development/configuration/single/export) and make sure the permissions are gone

Am I missing steps? Or does this only break when using Drush?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

And if possible some steps to reproduce this on a clean Drupal install. Just tried this on a clean Umami install and switching from /en to /es and that seems to work fine

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

The fault seems to lie with AccessController in the theme_permission module that is making this call, that needs to be updated to use the correct constructor arguments.

If you switch off that module, does the error go away? If so, please open an issue for that module.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

This feels like a configuration management issue from the first little dig I did, so moving it there for now. I don't think Workflows is doing anything wrong, seems more like Views is getting the wrong information, or Views is reaction to the information in the wrong way.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Yup, the new steps show the problem and the patch certainly helps see it! (strangely enough, it only shows the View getting deleted the second time I try to delete the workflow? Are others seeing that too?)

A little digging, \Drupal\views\Entity\View::onDependencyRemoval gets called with the editor role dependency, which is not getting removed but edited. But the View reacts to it like it's getting removed, and within the context of \Drupal\views\Entity\View::onDependencyRemoval it is impossible to determine if that dependency gets edited or removed. Since it's called 'onDependencyRemoval' I would assume it is only invoked when things get removed? But not sure.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

-1 from me too. The odds of somebody using it to 'fix' a problem without having a clue as to the consequences are just to big I think (same really goes for 'disable SQL rewriting', would not be in favour of adding that in either if it wasn't already there).

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

The current handler \Drupal\taxonomy\Plugin\views\argument\IndexTidDepth is very strongly tied to the node table and even states:

* This handler is actually part of the node table and has some restrictions,
* because it uses a subquery to find nodes with.

So, fair request, but non-trivial to implement I fear.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam
  • Used a clean(ish) Umami install, no contrib installed
  • Added a new workflow
  • Gave the authenticated user permissions for this workflow
  • Added the role based permission for authenticated users to a View
  • Deleted the new Workflow
  • View was still there

Am I missing steps? Can you reproduce this on a clean Drupal install?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Added a test, chose a Unit test in this case since we are messing with a base class. Setting this up for a functional test would maybe involve depending on Views since that is probably the only place this happens, which we don't want for a test for the menu system

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Hmm ok no idea why it failed on Drupal CI, but green now Β―\_(ツ)_/Β―

Just needs some test coverage than I guess.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Took another look.

The dropbutton render element expects links in the following format

[
        'url' => $path ? UrlObject::fromUri('internal:/' . $path) : $url,
        'title' => $title, 
]

So we can't pass it a fully rendered Views field, all the logic in AdvancedRender would needs to be redone in the Dropbutton plugin to format it in a way that works for a dropbutton.
In other words, the Dropbutton field in its current form is meant for simple implementations only.

I understand that this feel like a bug, and will leave it open as such, but it's close to being 'Works as designed'

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Option 3 sounds/looks good. Tried to reproduce the test fail locally, biut this test fails locally on HEAD too since it has a weird use of $base_path that doesn't work when running the test on '/'
When for convience I remove the base_path usage, the test is green. So lets see what happens on Gitlab first

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Added functionality and a test for it, totally plagiarised from Webform

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Works fine without the exposed filter, only breaks for me using the exposed filter first.

Uh use facets module? ;)

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

https://www.drupal.org/docs/8/core/modules/views/create-a-simple-page-view β†’ does sound like the right place to add to. https://www.drupal.org/node/1578582 β†’ is in a D7 book, so that seems like the wrong place.

Probably best to add a new page and add it there specifically for menu tabs, not sure it would fall in the 'Simple page view' category.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Did what @lauriii suggested. The other case in stable9.theme that seems to have done this didn't get test coverage as far as I can tell, so no test coverage for that for now

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Yeah that should work, just add that snippet in #17 to stable9

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Account settings form has a test, and that was failing due to the changes here, so updated that test with the new settings. Change to Account settings form for two fields that needed to use #config_target

Fixed the autowire test and added some type hints and constructor property promotion while I was at it

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Lendude β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

@smethawee the patch in #13 should let you get past the error (but the underlying break will still be in your site config), the patch in #27 will not fix anything but will hopefully make it clearer what part of your site is broken. And then you can fix that hopefully.

I haven't tried any of the patches so don't know if they actually help in their current state.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Updated the IS with the new proposal on how to address this.

I'll ping somebody to see if we can get some more opinions on how to deal with HTML class removal.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Well that answers that question :D
It seems to be pointless, but test coverage of this area of Views can be really sparse, so the green result doesn't mean too much I fear

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Very specific support questions are better asked on the support forums like https://drupal.stackexchange.com/

In this case you probably need to do this in a preprocessor function somewhere and not in the twig template, but no idea if this can be done for grouping.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Exposed filters can only search in 1 field at a time (with the exception of the Combined field filter but that only works for text fields normally, but if you want to search on the exact content of your date fields it might work)

If you are using something like Search API you could try using an Aggregated field or if you can write custom code, you could try writing a Computed base field that grabs all the data you need and then searching on that Computed base field.

But with just Drupal core, this can't be done like this.

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Removed the switching on of aggregation in an MR, let's just see what breaks!

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

So this just removes the BC layer. I don't see any way to detect that people are using these classes, all we can detect is if we are suppling them with BC classes, so raising a deprecation notice feel weird since it might not be relevant at all.

Would it maybe be sufficient to add a CR with a code snippet that could recreate the BC classes if somebody still needs them?

Something like this seems to do the trick:

function mytheme_preprocess_views_view(&$variables) {
  if (!empty($variables['attributes']['class'])) {
    $bc_classes = preg_replace('/[^a-zA-Z0-9- ]/', '-', $variables['attributes']['class']);
    $variables['attributes']['class'] = array_merge($variables['attributes']['class'], $bc_classes);
  }
  if (!empty($variables['css_class'])) {
    $existing_classes = explode(' ', $variables['css_class']);
    $bc_classes = preg_replace('/[^a-zA-Z0-9- ]/', '-', $existing_classes);
    $variables['css_class'] = implode(' ', array_merge($existing_classes, $bc_classes));
  }
}
πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

This still being in, is now leading to people opening bugs πŸ› Views CSS class is changed when underscores and dashes are mixed Closed: duplicate , lets see if we can get rid of this

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Doing what I said in #4

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

This is by design. As you indicate, there are workarounds for this, but changing it would be a feature request, just not sure what that feature would be.
Something like an option to add a setting for 'Use current URL' for blocks?

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Thanks for reporting this.

Do you have some steps to reproduce this on a clean Drupal install? Usually this is a symptom of a problem further up and just adding some checks is just hiding the real problem

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

the end results go against common sense expectations

Totally agree. But fixing this in a generic database agnostic fashion (default sorting between MySQL and PostGres is not the same) that would work for all the scenario's that require this (and detecting when it is required) and then figuring out which order clauses should apply to what, might be a bit of a challenge. But I might be overthinking this ;)

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

If/when πŸ› An infinite loop in ViewsJoin could cause denial of service Needs review lands we could probably throw a InvalidViewsDataException here.

I don't think we should do something like #13 because your site is in a broken state and something like that will leave it broken and just hide it (until the next thing breaks). So the exception direction in #27 sounds like the way to go for me, but would prefer the more specific Exception to be available

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Addressed feedback. Not sure if the minor scope creep for php-stan is better than updating the base line and fixing it later, but it seems pretty minor so went for the fix

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Just going through the code, this still works as it always has, it adds a destination link if there is none if you enable it, it would never remove destination links. So the change that added the text about not needing this anymore was #2767857: Add destination to edit, delete, enable, disable links in entity list builders β†’ where all the action links got destination parameters by default (and not just in Views).

So removing the destination parameters would maybe revert part of #2767857: Add destination to edit, delete, enable, disable links in entity list builders β†’ or we need to rewrite this plugin to start removing destination parameters.

The problem I see with using this plugin and start removing the destination parameters is that it would be impossible to keep the output the same as it currently is with the checkbox disabled, where some links can have a destination and some don't (and the default being FALSE and then stripping things sounds bad).

So dunno

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Added a comment explaining why we are doing this

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

The test was showing the reported whitespace, so lets trim that too

Production build https://api.contrib.social 0.61.6-2-g546bc20