Account created on 22 July 2015, almost 9 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom scott_euser
  1. Updated to 11.x
  2. Added updated hook + addressed feedback from #7

Thanks!

πŸ‡¬πŸ‡§United Kingdom scott_euser

scott_euser β†’ changed the visibility of the branch 3460063-views-display-extenders-filter to hidden.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Thanks for the work on this! Gave it a whirl and seems to be working correctly for me:

  • Changes in #45 seem to be addressed + match the way UseCacheBackendTrait does it
  • Running drush php:eval "\Drupal::service('entity_field.manager')->rebuildBundleFieldMap()" to check works fine
πŸ‡¬πŸ‡§United Kingdom scott_euser

Seems if you turn off table drag and turn on manual row weights, this patch flexbox causes an issue. Pushed a fix for that here.

Before fix:

After fix:

πŸ‡¬πŸ‡§United Kingdom scott_euser

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

πŸ‡¬πŸ‡§United Kingdom scott_euser

Confirmed this solves the issue:

Before:

After:

πŸ‡¬πŸ‡§United Kingdom scott_euser

Thanks for the review (directly on MR/Slack)! Merged into `ai-search`

πŸ‡¬πŸ‡§United Kingdom scott_euser
  1. Added test coverage
  2. Updated issue summary to standard template

Ready for review

πŸ‡¬πŸ‡§United Kingdom scott_euser

Okay https://git.drupalcode.org/project/ai/-/merge_requests/19 is ready now

Took a bit of refactoring to the Engine config to allow multiple sets of subforms so a fair bit of changes there as well. End result:

πŸ‡¬πŸ‡§United Kingdom scott_euser

Done in `ai-search` branch. Sorry didn't quite realise how we are using issues yet with the rapid development, so did this straight into the branches via gitlab.

git checkout ai-search
git diff 3e942b13..097b6c49

Shows the changes made, or browsing https://git.drupalcode.org/project/ai/-/commits/ai-search?ref_type=heads

πŸ‡¬πŸ‡§United Kingdom scott_euser

Refactoring done, configuration done, need to just apply it to the strategies now.

πŸ‡¬πŸ‡§United Kingdom scott_euser

This is in ai-search branch already and working fine supporting Cosine Similarity, Euclidean Distance, and Inner Product with Cosine set as default.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Added some commenting explaining, merged into `ai-search` branch

πŸ‡¬πŸ‡§United Kingdom scott_euser

Merged into ai-search branch thanks!

πŸ‡¬πŸ‡§United Kingdom scott_euser

Interesting one, good sleuthing! Will follow that issue as I'm curious where it leads.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Updated to match e.g. navigation.info.yml with both package and lifecycle

πŸ‡¬πŸ‡§United Kingdom scott_euser

Nevermind, this is actually fixed already on 1.0.x! Was just missing in ai-search branch. Merged into that

πŸ‡¬πŸ‡§United Kingdom scott_euser

scott_euser β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Phpcs failures already existing on main branch, so I think out of scope of this issue. The updated tests pass but fail without the code change as expected.

πŸ‡¬πŸ‡§United Kingdom scott_euser

For anyone to be able to take action you'd need to show doing it in a clean drupal 10.3 or 11 install with the latest version of the module(s) in question. Would suggest you start with e.g. just Core and this to limit any side effects. Others won't be able to narrow down your issue otherwise.

If you are able to code yourself, if you are confident it's this module, try comparing the viewsreference data in layout builder vs not layout builder in the viewsreference.module file's pre render and pre view methods e.g. with devel to see if you can spot the difference. Try also the two field formatters provided, maybe seeing if one or the other formatter solves (e.g. swapping in manage display), and again you could check whether the data there differs between working and not working.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Thanks everyone for the work on this! To help review it would be good to have a single branch, and other branches hidden as its unclear at the moment (+ mix of paches).

Looking at 2985364-pass-token-from-paragraph branch, it would be good for someone to explain better to me the aim here. From the issue summary, it seems it should be isolated to the ViewsReferenceArgument setting, but all settings are changed? So maybe issue summary needs an update, or scope needs to be more limited.

Now that we have test coverage everywhere, any merge requests coming in would also need to have test coverage please.

Thanks!

πŸ‡¬πŸ‡§United Kingdom scott_euser

Some progress adding a test, need to wait for ajax request, but out of time for today. Hopefully at least it gets someone going further on this (or hopefully I can come back to it soon enough)

πŸ‡¬πŸ‡§United Kingdom scott_euser

Confirmed that the tableExists method now correctly matches the parent method in web/core/lib/Drupal/Core/Database/Schema.php:

  public function tableExists($table, bool $add_prefix = TRUE) {

RTBC +1

πŸ‡¬πŸ‡§United Kingdom scott_euser

For anyone coming across this still having 414 issues, there is now https://www.drupal.org/project/viewsreference_extras β†’ which reloads the views reference configuration from the entity instead of compressing the configuration as is.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Thanks! For anyone coming across this still having 414 issues, there is now https://www.drupal.org/project/viewsreference_extras β†’ which reloads the views reference configuration from the entity instead of compressing the configuration as is.

πŸ‡¬πŸ‡§United Kingdom scott_euser

scott_euser β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom scott_euser

I just tried to reproduce this and my pager seems to work fine. Any JS console errors or anything in /admin/reports/dblog for example?

πŸ‡¬πŸ‡§United Kingdom scott_euser

Apologies for the silence here. We have progressed a fair bit with the module since that point in time, its very likely this is no longer an issue. Could you reopen if you still experience the error?

Thanks!

πŸ‡¬πŸ‡§United Kingdom scott_euser

Can I check if you are on the 8.x-2.x dev branch and you have https://www.drupal.org/project/viewsreference/issues/3396530 ✨ Views reference results in 414 if many options set and ajax used Needs review in your code? I think it might be related, spotted the regression, test coverage not good enough, just working on solution now.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Added a few lines of test coverage, without your code change, test fails to see both css files in place, with the code change it sees them both as well.

Thanks for spotting this!

πŸ‡¬πŸ‡§United Kingdom scott_euser

Makes sense and looks good to me! I'll just give this a manual test run in the upcoming days and get this merged in. Thank you!

πŸ‡¬πŸ‡§United Kingdom scott_euser

Okay added back in field_api_classes - confirmed that the test still passes in πŸ› Views handler loading should respect configuration Active with this.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Okay @alexpott helped solve that extending here https://drupal.slack.com/archives/C1BMUQ9U6/p1720626481941179 (thank you!).

I tested via πŸ› Views handler loading should respect configuration Active running core/modules/views/tests/src/Functional/Entity/BaseFieldAccessTest.php

  • Without any `views.field.field_language` schema: test fails
  • With it in place and `type: views.field.field` extending: test passes
πŸ‡¬πŸ‡§United Kingdom scott_euser

The 'Test-only changes' confirms this successful fails without the array_filter code change. Ready for review.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Is there any example we could follow here? I guess we need to validate that the string in views.settings.yml > views.settings > mapping > display_extenders is a valid plugin.

Similar to:

            constraints:
              PluginExists:
                manager: plugin.manager.views.display

Except in this case we do not have a plugin manager, so perhaps actually the first step is to create a plugin manager for them?

At the moment they are fetched in core/modules/views_ui/src/Form/AdvancedSettingsForm.php via Views::fetchPluginNames('display_extender') for example.

πŸ‡¬πŸ‡§United Kingdom scott_euser

This issue blocks πŸ“Œ Add validation constraints to views.settings Postponed I believe. So once this gets sorted, that one get go back to NR.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Okay I created πŸ› Views display extenders should only save enabled extenders Needs review to prove that display_extenders issue + sort it with the array_filter. We are already filtering them on load in Drupal\views\Views::getEnabledDisplayExtenders() so it seems logical to filter them out on save. To avoid a more complicated update, I think it does not hurt to keep the array filter in the ::getEnabledDisplayExtenders().

So I think this is postponed until πŸ› Views display extenders should only save enabled extenders Needs review gets merged right?

πŸ‡¬πŸ‡§United Kingdom scott_euser

Added the array_filter + test coverage to ensure they get filtered. If you run the test on its own without the change, it fails, showing that the issue exists.

πŸ‡¬πŸ‡§United Kingdom scott_euser

scott_euser β†’ changed the visibility of the branch 3440962-add-validation-constraints to hidden.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Spun off issue here 🌱 [meta] Decide how to manage test view configuration so it remains up to date Active . Will try to get my head back into this for sorting out this particular issue.

I guess continuing is probably better than waiting to figure out a way to automatically solve it as it is probably going to be complicated as the Views config will change depending on which other modules are enabled at the time. We could look at module/view dependencies, but as your diff shows, we cannot trust that the view config dependencies were correctly set up in the first place either.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Sorry this ended up being so trivial. I guess having thoroughly tested and added automated tests via a Drupal 11 default install paid off here. Should at least help for eventual Drupal 12.

πŸ‡¬πŸ‡§United Kingdom scott_euser

It seems unlikely this will get in, so would suggest others avoid this and consider πŸ› Views reference is adding many query string variables to the pager URLs RTBC instead. I may consider adding a functionality similar to this in a separate module. Will update here if so.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Okay after discussion with @seanB in Slack:

  1. Removing the reloading the settings directly from the entity when the compression query string is still too long and will shift that to a new separate module.
  2. Converted to a service so that new separate module can then handle larger compression via reloading from the entity.

For those coming across this issue after it is marked as Fixed here, if you still have this issue, please see this module homepage for a link to the extra module.

πŸ‡¬πŸ‡§United Kingdom scott_euser

I wonder if its also worth having an update hook to update the views config of existing sites if its unchanged?

πŸ‡¬πŸ‡§United Kingdom scott_euser

Thanks for the feedback on the progress so far! Replied to comments, will continue to work on this

I also added the related issues that will block this issue.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Not sure if there is a way for the schema of views.field.field_language to just inherit everything from views.field.field? It would make sense to inherit if we can since the 'Language' field plugin extends 'Field' field plugin with nothing other than an access control method (so that the field does not show up in a monolingual site).

πŸ‡¬πŸ‡§United Kingdom scott_euser

Thanks for flagging, updated to 11.x (issue & MR)

πŸ‡¬πŸ‡§United Kingdom scott_euser

scott_euser β†’ changed the visibility of the branch 3458312-overrride-comment-views-handler to hidden.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Searching around I cannot see examples any more of it in the docs. The only thing I could find is a note about it here: https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... β†’ if you control+f for "deprecated.sequence.definition.format:". Ie, it looks like the documentation is correctly updated.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Its definitely the former; for example in one of my modules, I had the type set to sequence and had to change it to config_object + adding mapping https://git.drupalcode.org/project/unified_date/-/commit/b40d053b77e3f24...

It certainly seems that this is a common problem (maybe we are all copy pasting from each other or maybe there is an outdated doc page - will try to unpick that - but I do not think it should block getting this in though). When you see the error + know the config name, a quick search picks up many similar results allowing the developer to sort the issue. Some more examples of results that come up for the same issue as the above with unified_date:

  1. πŸ› Config schema is incorrect Active
  2. πŸ› AssertionError: assert($typed_config instanceof Mapping) in assert() Active
  3. πŸ› Config schema is incorrect Active

I expect this issue will show up in a search as well giving hints as to how to solve. Without the config name indicating the failure though, its much harder for the developer to know which module to look at.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Okay this solves it, if anyone stumbles across, careful around very specific preprocess functions; MODULE_NAME_preprocess_node_pdf_content__type was interfering with this in testing and changing to MODULE_NAME_preprocess_node_pdf_content then handling the type solved.

πŸ‡¬πŸ‡§United Kingdom scott_euser

Going to postpone this until we get things pluggable first πŸ“Œ Refactor to allow multiple puppeteer services Active

Production build 0.69.0 2024