🇬🇧United Kingdom @scott_euser

I took a look at the logic and the subsequent code really does need the #name - if its missing every bit of the subsequent code falls apart as its essentially the key bit to determine the dependee. So it seems safe to me to merge it @dqd

Moved the patch to an MR for ease of merging, but no change so marking as RTBC

scott_euser → made their first commit to this issue’s fork.

scott_euser → created an issue.

This does solve it yes thanks! But it also needs a version bump since the version is hard-coded rather than using VERSION as per the warning at https://www.drupal.org/docs/develop/creating-modules/adding-assets-css-j... →

As this is a trivial change, considering this as RTBC

scott_euser → made their first commit to this issue’s fork.

Okay I have made changes now, sorted all eslint errors, fixed typos.

The phpstan issues are unrelated to this MR, they are new to the module and therefore raised separately.

To help progress this issue if someone could please:

1. Review and test 🐛 Fix deprecated extending of render element Needs review to RTBC it so tests here can go all green (including phpstan)
2. Re-review this consider the changes I made to eslint/typos to get this back to RTBC

Then we'll hopefully all get the satisfaction of getting a 6 year old issue over the line :)

Here is the change record from Core https://www.drupal.org/node/3436275 →

scott_euser → created an issue.

Okay I believe credit list is also now up to date up; tried to also help maintainers by reviewing all people who have made contributions matching/exceeding the guidelines (ie, not just 'it works' type comments, meaningful reviews, code contributions, etc)

Bit of a mess between patches & merge requests & outdated merge request fork.

Now the remaining MR is actually merge-able and contains identical content to the patch in #124. As such, marketing this as RTBC as I haven't actually made any change - just did a bunch of issue clean-up - and testing this it works as expected.

For future travellers please follow https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → and download the patches locally from the MR to your project as per the recommendations/instructions rather than adding more patches here and having patches and MRs diverge again. Ie, let's keep us all moving forward on the same track and make life easier for the maintainers of this module to review & merge. Thank you!

scott_euser → changed the visibility of the branch 2969051-html5-validation-prevents to hidden.

scott_euser → changed the visibility of the branch 8.x-3.x to hidden.

scott_euser → made their first commit to this issue’s fork.

Anyone coming across this, if you have specific issues, please create new issues with:

1. The source html getting paste from the clipboard
2. The software you are using
3. Ideally a merge request looking at the commit above to see where the transformation is happening

Thanks!

Thanks Sam! Got this going so it also now happens before the 'Limit allowed html' strips out anything not allowed. Its going to need some more testing in reality as there may be edge cases or software that does not work, but at least having the foundation in here will be a good start.

Also fixed eslint issues

So what do you think is in scope? Current search_api_ai module does the mix as a data type

scott_euser → created an issue.

scott_euser → created an issue.

Howabout when you set up the Search API Index you select:

1. Vector Storage plugin - ie, what is currently in open_ai_embeddings module
   1. Pinecone
   2. Etc
2. Content Chunking plugin - new, selectable when configuring search api
   1. Minimum chunks - index the entire content with as few chunks as possible https://git.drupalcode.org/project/search_api_ai/-/blob/1.0.x/src/TextCh...
   2. Chunk per field - index each field separately, chunking within that if needed https://git.drupalcode.org/project/openai/-/blob/1.0.x/modules/openai_em...
   3. View mode chunks - select one or more view modes, each creating a set of chunks

Each chunking plugin has a getMetaData() method, returning keyed array

So meta data for each might look like

Added to each chunk

• minimum_chunks <--- or whichever chunker
• search_api_index_id:my_index_name
• search_api_datasource:entity:node
• search_api_item_id:entity:node/1000:en

Minimum chunks, 3 example chunks

• minimum_chunks:plain_text:0
• minimum_chunks:plain_text:1
• minimum_chunks:plain_text:2

Chunk per field, 5 example chunks

• chunk_per_field:title:0
• chunk_per_field:field_body:0
• chunk_per_field:field_body:1
• chunk_per_field:field_body:2
• chunk_per_field:field_additional_info:0

View mode chunks, 5 example chunks

• view_mode:teaser:0
• view_mode:summary_info:0
• view_mode:summary_info:1
• view_mode:bio_info:0
• view_mode:bio_info:1

Regarding this:

2. Add the embeddings rule as a variable for the chunking process, since this holds important data like max input size and is also where the embeddings call is triggered.

This would just be the vector storage plugin + the configured number of dimensions for the chosen LLM (like I was adding in ✨ Support new embeddings models and dimensions (3-small, 3-large) Needs review )

So, when configuring search api, the site builder must:

1. Choose their dimensions from the options available in the LLM
2. Choose their vector storage
3. Choose their chunker

If any of these changes, flag for re-indexing.

Thanks for flagging! Can you create a merge request with your patch please so tests run/coding standards run/etc? Thanks!

Nvm #4 not addressed, needs work

Patch solves the issue correctly as it matches the conditions where an InvalidArgumentException would get thrown to prevent those conditions from being met and resulting in the fatal error. Ie, here is the excerpt from the ::fromUserInput() method

if (!str_starts_with($user_input, '/') && !str_starts_with($user_input, '#') && !str_starts_with($user_input, '?')) {
  throw new \InvalidArgumentException("The user-entered string '$user_input' must begin with a '/', '?', or '#'.");
}

Thanks! Going to quickly convert it to a merge request, but since I'm not making any changes, just facilitating the maintainer's job, suggesting RTBC as status.

scott_euser → made their first commit to this issue’s fork.

It would be good to update the homepage; it seems what this module says is unique to it (translations bit) is covered by Scheduled Transitions (a more well used module). So maybe its more of a duplicate than it used to be.

Could use some suggestions on how to add tests for this (if its possible for the install)

scott_euser → created an issue.

Great stuff, thank you! Did a round of manual testing as well and all working as expected. Thank you for adding the test coverage as well.

Thanks!

So current situation with this MR is now:

1. 'Include the view title' plugin not enabled on the field: No title shown
2. 'Include the view title' plugin is enabled on the field: Checkbox unchecked: No title shown
3. 'Include the view title' plugin is enabled on the field: Checkbox checked: Title shown

Patch should work now. Could you please confirm?
Test coverage also added.

Thanks for the work on this! A few things:

1. Can this be converted to a merge request to make it easier to review and comment please? + so tests run
2. This needs test coverage, which can be added to ViewsReferenceSettingsTest.php

Thanks for the work on this! A few things:

1. Can this be converted to a merge request to make it easier to review and comment please?
2. The last unset($argument) is actually deleting the argument is it not? Since it is passed by reference.
3. This needs test coverage, which can be added to ViewsReferenceSettingsTest.php (there is an existing method ::testArgument())

Aha I see, typically needs review is meant to have something to review, hence my confusion. Setting back to active.

Note that there also seem to be related issues 💬 Is there any way to pass the token from the current paragraph to the view? Needs work and 🐛 Multiple contextual filters / arguments are not supported when a value is not provided for them Needs review that you could try. This could possibly be a duplicate, though I have not tested yet myself.

Added a bit more test coverage to ensure the unified_date field is actually getting picked up by the token as well. Manual testing also working fine still.

I think assuming all tests pass now, should work well

Yeah I can see that being useful, thanks for flagging. Copy pasted across from node:created, seems to work fine. Gave it a manual test myself via metatag module just to check and seems to work okay + provide the full list of date formats like node:created/node:updated do. I don't fully get the bubble meta data use that Core's node.tokens.inc uses, but kept it in to avoid issues.

Does this look okay to you @heddn?

scott_euser → made their first commit to this issue’s fork.

I would also be okay if you wanted to create a seperate permission for configuring like 'administer site settings types' but this would then also need an update hook to maintain the status quo for existing installs.

Thanks for raising. However we should not give create, update, and delete access. Only view access should be allowed. So admin permission attribute would need to be removed and replaced with an access handler so existing permission can be returned for all but view.

Thanks!

--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 77 | ERROR | [x] Parameter $options has null default value, but is not marked
    |       |     as nullable.
    |       |     (SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue.NullabilityTypeMissing)

I would have thought that https://git.drupalcode.org/project/drupal/-/merge_requests/4053/diffs?co... would fix this. Maybe I did the wrong thing, would appreciate if anyone has any tips. It seems there is a giant inheritdoc chain and this MR is just surfacing a deeper issue elsewhere.

I definitely had a bit of hair-pulling as well, what I ultimately did was take the merge request diff as a patch and ran the apply patch via phpstorm which applied most files. Then I manually redid the views.views.inc as it could not manage that. So yeah, no sane way I could find yet either :)

scott_euser → made their first commit to this issue’s fork.

Okay I need to work on reproducing this without Views Reference Field module, a bit down my queue though unfortunately.

On the plus side, have managed to get a lot more test coverage into Views Reference Field module itself on (I assume everyone here is here because of some combination of that module and/or Views Ajax History) so hopefully welcome news!

scott_euser → changed the visibility of the branch 11.0.x to hidden.

scott_euser → changed the visibility of the branch 10.1.x to hidden.

scott_euser → changed the visibility of the branch 10.3.x to hidden.

Okay added test coverage as well to help get this in faster.

For now I pulled all the latest changes from 2x into this merge request so tests start running as well on it.

Thanks for the work on this! Could someone update the issue summary with steps to reproduce please? I believe I should check out a particular version of the 2x branch, set up a Views Reference Field with a particular set of settings, then update to latest 2x branch and see the incorrect configuration saved.

I agree with #6 here; when the negate is in place, instead of a key diff, we should load all available Views excluding the ones selected.

A few tasks here:

1. Convert to Merge Request
2. Make the field label match the standard set in core, i.e, 'Negate the condition'
3. For both Select and Autocomplete, we should ensure that all Views except those selected are allowed
4. Add basic test coverage

Thank you!

Hmmm looking at that issue further, I think I am probably wrong. I will try to get to testing this as soon as I can to help improve the upgrade path. Thanks!

Thanks for the details! I believe this is actually a duplicate of 🐛 Add missing update hook for adjusting viewsreference field settings RTBC - @e5sego would you be able to check and confirm there if the upgrade hook covers your case as well please? If I have this wrong and its a separate issue, apologies!

scott_euser → made their first commit to this issue’s fork.

Test only MR, so merging on my own as discussed. Thanks!

RIght, makes sense!

Test only MR, so merging on my own as discussed. Thanks!

scott_euser → changed the visibility of the branch 3396530-views-reference-results to hidden.

Okay that's implemented. Opted for also compressing the reload of the options to make it less tempting for someone to want to modify the options in the URL + checking entity access on the reloaded entity.

@seanB if you are happy with this approach I'll get some test coverage properly in for it (right now its not covered).

Hmmm I think actually state becomes problematic if someone shares a particular URL. Seems better to implement this suggestion from #7:

We could theoretically also just pass the parent entity type, ID/revision ID and langcode. We could then load the data in the AJAX request when we need it? This could potentially break any alters etc though.

Views Ajax HIstory maintainers merged both my issues quickly, tests now pass

Okay proof of concept here: this sorts it for me with unlimited length URL, BUT it does not work with compression alone. With compression we get a lot of the way there, but the URL limit according to https://stackoverflow.com/a/417184 should be 2000. If we leave space for exposed filters and other bits in the URL, with a lot of possible configuration options, it still gets hit.

I suggest we default to compression, and fallback to state if that fails. Would like to get other maintainer buy-in on this before I:

1. Add test coverage properly for this (ie, I just grabbed my tests from Views Ajax History just for quick POC)
2. Make the threshold of switching to state configurable or override-able somehow?

Thank you!

Makes sense, thank! Will give this a shot. Existing functional JS test coverage should help here.

scott_euser → created an issue.

Thanks for reviewing; thread 1 resolved, thread 2 showed screenshots of what submitForm() does under the hood which is actually filling in the fields and clicking the button, its more of a helper function to avoid having to repeat that code across tons of tests.

We definitely still have this issue even with 📌 Compress ajax_page_state Fixed . We are mostly using 🐛 Views reference is adding many query string variables to the pager URLs RTBC though to solve this rather than this patch though. This patch is opinionated, whereas the other approach gives the control to the developer, so perhaps more likely to get merged? Needs maintainer opinion.

Do you know which specific settings you had to enable? Seems we should modify the update hook in the install file to better maintain the status quo between 1x and 2x branches?

I don't see 'Show filters on page' in the code, is it perhaps a custom Views Reference Settings plugin you are using?

Tried to make the summary easier to skim

Updated with latest features of Views Reference 2x features

I am not sure if I am doing Viewfield or Views Field View justice, so please edit if you feel I missed something. It seems these two modules offer a simple way to render a View via a field with comparable minimal configuration. Then a dev should choose Views Reference Field if they want more advanced options + the ability to extend via the plugin system to allow override of far more options.

Great thank you!

Thanks for the quick action, much appreciated!

Tests are going to fail without Views Ajax HIstory solving those two bugs, but tests pass locally with those patches applied.

Also had to raise 🐛 isArray is removed in Drupal 11, causing a fatal error in this module Needs review as Views Ajax History is broken in Drupal 11.

scott_euser → created an issue.

Had to raise 🐛 Exclude args is missing schema Fixed in Views Ajax History or tests will fail on schema being invalid.

Thanks for adding this feature, it has been very useful! Unfortunately it introduced a schema regression, raised as a follow-up here 🐛 Exclude args is missing schema Fixed .

scott_euser → created an issue.

Extremely trivial change, but at least for me I find gitlab integration with drupal.org makes merging these trivial changes extremely quick via UI at least.

Thanks!

scott_euser → created an issue.

scott_euser → created an issue.

Test only without code change correctly fails.
Test coverage therefore shows this is an issue + shows that the code change solves it.

Ready for review.

Won't this then apply also to non-facets? Ie, if I:

1. Create a view with exposed filter for a taxonomy reference field
2. Use better exposed filters to set that exposed filter to display as a series of checkboxes
3. Click a checkbox using the view

If the Views Exposed Form is not using auto-submit, I would expect it to not add to the history state until I click apply. So I believe this code needs to opt-in via the Facet settings?

Thanks for flagging. I tested creating a view as per the issue summary and indeed the display IDs fail to load as the preg_match greedily gets all available words in brackets. Updated code to just get the last word in brackets - ie, the machine name of the view.

scott_euser → made their first commit to this issue’s fork.

I don't see a patch or MR, so I think its maybe wrong status.

Making display types required as per the MR makes sense; you must select at least one of these:

Marking this as RTBC.

For #13 it sounds like a separate issue, might be worth opening with steps to reproduce from a clean install including the settings you used.

See my comment on the MR + now that we have tests, the tests assume the default behaviour, so we should update the test coverage to cover a View with 2+ displays and a View with only 1 display.

scott_euser → made their first commit to this issue’s fork.

Okay test coverage for all built in Views Reference Settings now. Requires merging 🐛 Add functional coverage (without needing JS), support using the module without JS Needs review first though as these tests rely on Views Reference being usable without JS.

scott_euser → changed the visibility of the branch 3453902-settings-test-coverage to hidden.