Looks like it's a random fail ๐ [random test failure] ScaffoldUpgradeTest and ScaffoldTest rely on packagist.org Active
Looks like the test fail is an unrelated PHP 8.5 test.
Rebased on 11.x and fixed tests. We had space ' ' in the list of special chars, but this is handled separately.
kim.pepper โ created an issue.
Ran into a bit of an issue with the pipelines. In order to have Opensearch generate the text embeddings, you need to specify text field to embedding field mappings when creating the pipeline. I don't think it would be easy to dynamically create a pipeline like this with search api.
I'm going to check out the https://www.drupal.org/project/ai_vdb_provider_opensearch โ module to see if the built-in AI Search would work.
Started work on a more integrated approach. At this stage all the MR does is set index.knn = TRUE
when creating the index.
In order to have knn enabled on an index, we need to set that option when creating the index. We can change it after.
This meant we needed to refactor the addIndex()
method to not create then update settings, but to pass the settings at creation time. This refactoring could potentially be split out into a separate issue.
I reviewed the changes and confirmed that all use of FileSystemInterface::basename()
have been converted to basename()
.
My only question is on which version this will be removed in.
@deprecated in drupal:11.3.0 and is removed from drupal:13.0.0.
Is it too late to have it removed in drupal 12.0.0 ?
Looks like you are correct. I'd be happy to deprecate/remove.
Confirmed that:
- the bug linked in the comment is no longer relevant.
- Directory separators are handled natively
- The behavior of php basename() seems to handle directories ending in slash just fine.
* PHP's basename() does not properly support streams or filenames beginning
* with a non-US-ASCII character.
@see http://bugs.php.net/bug.php?id=37738
Here's an 3v4l snippet to test the standard basename() behavior:
Committed to 3.x and 2.x. Thanks!
Updated to catch both OpenSearchExceptionInterface and ClientExceptionInterface
Hmm. What version of opensearch-php
are you using?
We should be able to just catch OpenSearchExceptionInterface
as we convert any HTTP responses with status >= 400 to an instance of \OpenSearch\Exception\HttpExceptionInterface
which extends \OpenSearch\Exception\OpenSearchExceptionInterface
.
Thanks for reporting. We actually need to remove \OpenSearch\Common\Exceptions\OpenSearchException
as it is deprecated in opensearch-project/opensearch-php
2.4.0
Mohit and I discussed this in Slack and he agreed for me to post this request.
kim.pepper โ created an issue.
xjm โ credited kim.pepper โ .
A recommended approach for vector indexing is an ingest pipeline. I wonder if this issue could be expanded to include support for that?
Create an MR against 3.x and it will get cherry-picked into the 2.x branch.
Thanks for reporting this issue. Can you please create a MR for it so automated test pipelines will run?
I reviewed this issue as part of Bug Smash Initiative.
I did not try to reproduce this locally. It looks like the code is almost identical to when the issue was posted.
Unfortunately there is no context to that change to inform whether it is intended behavior for security reasons.
It would be helpful to have an automated test to prove the bug exists.
I'm changing to PMNI to get feedback from the OP and see if it is still occurring.
griffynh โ credited kim.pepper โ .
Hiding files
smustgrave โ credited kim.pepper โ .
This should only go in 2.x as we want to remove it in 3.x
I created a follow-up issue to restore the orginal behavior. ๐ Restore querytime_synonyms analyzer Active
kim.pepper โ created an issue.
It's been 3 years. Any appetite for a 2.x branch to begin removing deprecated code and adding typehints?
Committed to 2.x and 3.x. Thanks!
kim.pepper โ created an issue.
I reverted the autowiring of SearchApiConverter as the BC logic in there was too complex. Created a follow up for that ๐ [PP-1] Autowire SearchApiConverter service Postponed
kim.pepper โ created an issue. See original summary โ .
kim.pepper โ created an issue.
kim.pepper โ created an issue.
I don't think it qualifies as a bug if you manually put in a url with missing params.
Updated the issue summary and added a change record.
Oops! Not sure how to best manage multiple analyzers.
I realize this will conflict with ๐ [pp-3] Split up and refactor system_requirements() into OOP hooks Active is therefore postponed on that, but wanted to make a start to see what changes are needed.
Working on this.
๐ Add value objects to represent the return of hook_requirements Needs work is blocked on this.
I pushed up my work in progress in case it's useful later.
Damn. I wish I saw your comment before I started converting system_requirements()
๐
Looks like drupal_requirements_url() is the only caller of drupal_current_script_url() so both of these can be deprecated. I don't think putting them on the RequirementSeverity enum is the right place. These functions are used by the install script to pass query params around.
Rebased on 11.x
smustgrave โ credited kim.pepper โ .
Reviewed the changes. Looks nice and simple.
kim.pepper โ created an issue.
kim.pepper โ created an issue.
I think this can be closed as ๐ Create enums for RequirementSeverity and RequirementPhase Active went in which deprecates the REQUIREMENT_* constants.
Yeah i don't think we want to do any of that on the Drupal side.
Manually tested and confirmed adding the synonyms filter to the default analyzer fixes this issue.
Committed to 2.x and 3.x. Thanks!
Committed to 2.x and 3.x. Thanks!
Should just be 'verify' => false
according to the docs. https://docs.guzzlephp.org/en/stable/request-options.html#verify
As for the exception in #2, that just looks like an authentication error?
FYI the docs say quotes for array keys in array shapes aren't needed but I added them anyway https://phpstan.org/writing-php-code/phpdoc-types#array-shapes
Thanks for the reviews. Resolved all threads.
Do you mean constructor params? This may just be the limitation of the underlying call to stream_wrapper_register()
which just takes a class name. See https://www.php.net/manual/en/function.stream-wrapper-register.php
Adding a related issue saying we should prevent duplicates โจ Drupal should prevent uploading of duplicates for managed files Postponed: needs info
Actually this is a feature request.
Ran the test-only job and confirmed it fails.
Should we close this issue in favour of ๐ Create enums for RequirementSeverity and RequirementPhase Active ?
Still a few unsresolved comments on the MR
Rebased on 11.x
I also switch the file install config to replace_whitespace: true
. I'm not sure this is sufficient as a default or whether we want to go the extra step and enable it via an update hook. This will be disruptive as per #2
Feedback has been resolved and all tests are green ๐ข. Back to NR.
Looks like \Drupal\package_manager\ValidationResult
was created using int values in
โจ
Add Alpha level Experimental Package Manager module
Needs review
before this could get commited. ๐ Created a follow up
๐
[PP-1] ValidationResult should use RequirementSeverity enum
Active
kim.pepper โ created an issue.
Added a CR about the new setting.
I'm working on rebasing this. Now most of the hooks are converted to OOP we can see what is left over.
kim.pepper โ changed the visibility of the branch 11.x to hidden.
greggles โ credited kim.pepper โ .
greggles โ credited kim.pepper โ .
Rebased on 11.x.
I think this might get more attention if it were under theme system.
Looks like permissions aren't set correctly in your file system. Did you follow the setup instructions here? https://www.drupal.org/docs/administering-a-drupal-site/security-in-drup... โ
Changes should be against 3.x and need to be in a MR to trigger the CI pipeline.
Looks like some legit test fails.
Rebased on 11.x
kim.pepper โ made their first commit to this issueโs fork.
Updated title.
Updated IS to reflect latest proposed resolution.
Added a default mime type constant of \Drupal\Core\File\MimeType\MimeTypeGuesser::DEFAULT_MIME_TYPE
with value 'application/octet-stream'
and use that instead of empty string.
Hiding some older patches and MRs.
kim.pepper โ changed the visibility of the branch 3469280-drupalfileiconmimetypes-doesnt-handle to hidden.
kim.pepper โ changed the visibility of the branch 3469280-File-mime-type-null to hidden.
Changing from a bug to a feature request.
I think this is a performance improvement rather than a bug so changing to a task.
Tagging as needs issue summary update to add in the template, and rename title to replace drupal_move_uploaded_file()
which no longer exists.
According to
https://www.drupal.org/about/core/policies/core-dependency-policies/depe... โ
we need framework and release manager signoff on adding a new dependency on psr/clock
. ๐คท
Rebased on 11.x Tests green again.