🇺🇸United States @afinnarn

Looks good to me! The one functional JS test passes, and I don't see any errors.

There are some suggestions in the warnings that can be fixed in future issues, but I see no reason to add CI now so we know if any change breaks the one lonely test.

Also to note, "DrupalFinder" is saying the current rector.php file uses deprecated methods.

  $drupalFinder = new DrupalFinder();
  $drupalFinder->locateRoot(__DIR__);
  $drupalRoot = $drupalFinder->getDrupalRoot();

From the website https://github.com/webflo/drupal-finder, they recommend this code now:

$drupalFinder = new \DrupalFinder\DrupalFinderComposerRuntime();
$drupalRoot = $drupalFinder->getDrupalRoot();

I will leave this at RTBC since I confirmed that once the patch is merged, it will update swagger-ui to 5.x. For now, I had to do the following:

Update composer.json repositories section:

    "repositories": {
        "drupal": {
            "type": "composer",
            "url": "https://packages.drupal.org/8",
            "exclude": [
                "drupal/openapi_ui_swagger"
            ]
        },
        "drupal/openapi_ui_swagger": {
            "type": "git",
            "url": "https://git.drupal.org/issue/openapi_ui_swagger-3409145.git"
        }

Then run these commands:

1. composer remove drupal/openapi_ui_swagger --no-update
2. composer require drupal/openapi_ui_swagger:dev-3409145-update-swagger-ui#bd0b5aa612c5c49763e28f8d19dfef9940166f49 -W

Now when you look, you should see swagger_ui 5.x loaded instead of 3.x.

Hey @vipin.mittal18, are you getting the 5.x library with this patch? I thought I was, but when I look in the libraries directory, I still have 3.x . I think the patch might be applied after Composer installs and merges dependencies, but I'm not sure. I will continue to look into this and maybe try pointing Composer to the MR branch.

If you aren't getting 5.x either, then maybe this MR has to be merged for us to pick it up or at least the issue should go back to "Needs Work".

I just deleted the `swagger_ui` library in my libraries directory and re-ran `composer install` which correctly placed the library again. I will post what I have in my composer.json.

        "installer-paths": {
            "docroot/core": [
                "type:drupal-core"
            ],
            "docroot/libraries/{$name}": [
                "swagger-api/swagger-ui",
                "type:drupal-library",
                "type:bower-asset",
                "type:npm-asset"
            ],

Here you can see "swagger-api/swagger-ui" added before the other installer path entries for "docroot/libraries". I'm not sure if that helps or not, but I had no problems with following the directions as they are written now in the documentation.

Adding a patch since I've seen people do this with MRs to reference...

This MR includes a "Field Types" form adjacent to the resource overrides, allowing users to select which field types a default enhancer can apply. Individual field instance enhancers still override the default field type enhancer.

Originally, I listed out all the field types so users could choose to add enhancers in one big form, but the way I was doing it I ran into `max_input_vars` issue on my work QA server. So, I switched to forcing users to declare the field types they wanted to include, which is probably better anyways since most people would only want to use this on a few field types.

I think the form and form language could be improved, but the approach of adding to FieldItemNormalizer and checking an additional config made sense to me. However, feel free to suggest whatever changes make sense...also where to add tests/what tests would be helpful for this change as I am not super experienced with PHPUnit.

I did not add the NULL enhancer, but that wouldn't be much work to add in this pattern.

Also, I have a disabled key in settings that I didn't implement, but it would disable all field type enhancers only allowing the field instance overrides. The benefit would be mainly for debugging where you can keep your settings but have a flag to turn off the whole field type enhancer feature.

From the Kernel tests, I keep seeing this failure repeated, but I don't think this code change has anything to do with something in the jsonapi module.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "Drupal\jsonapi\Controller\TemporaryJsonapiFileFieldUploader" has a dependency on a non-existent service "file.validator". Did you mean one of these: "form_validator", "path.validator", "email.validator"?

I also saw deprecations in the test logs, but other tests passed when I ran them.

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

Just popping in to highlight the JSON:API spec and section on includes: https://jsonapi.org/format/#fetching-includes

So the main question seems to be whether Drupal's JSON:API implementation should return an error if you try to request an invalid relationship across all resource types returned from the endpoint or just return an empty array for resources that don't include the specified field. Rather than see an error, I've wanted more information returned when I've run into this situation, so I'm guessing most Drupal developers would like to see this change.

Either approach appears valid, but who/what/how do decisions like changing the current implementation at a higher level get discussed and resolved? I'd think that would require some other issue or change record beyond this issue, but I'm not sure.

Well, I wanted to add another patch but https://git.drupalcode.org/project/entity_field_fetch/-/merge_requests/3... now has two patches one from each commit. I expected the patch to just cover the changes, but I guess it respects the commit order.

Oh well, the MR appears to render the values without using the value property. So, the MR is probably the way to go and simply deletes the TypedData field property classes and instead sets the values in setValue like before.

Adding a patch putting things in the value property...I will add a patch keeping things out of value next...

afinnarn → created an issue.

Turns out that most core field types I've looked at don't provide unit tests, but rather they test at a higher integration level using KernelTestBase.

I am looking at how the LinkItem and PathItem classes are tested, and I think this will turn into tests where you create a content type, add the field, save a node with the field filled in...blah blah. I, of course, need to create an entity to fetch data off of first, but at least kernel tests don't usually have you mocking anything. I still get caught up with mocking/stubbing/spying and all that jazz.

afinnarn → created an issue.

afinnarn → created an issue.

afinnarn → changed the visibility of the branch 3300098-entity-field-fetch to active.

I checked out the MR and tested it on an existing site and a new Drupal 10.2 install. All the testing steps passed, but I didn't test the GraphQL part on the new Drupal site I created since I've never set that up. The GraphQL output worked on my existing site. Also on the new Drupal 10.2 install the fetched field still saved the db value as "0".

I think the MR looks good to me. I'll put some comments below but not anything to prevent merging.

1. I've seen "setClass('Drupal\entity_field_fetch\PropertyTargetType')" done more like "setClass(PropertyTargetType::class)" with adding an addiitonal use statement. I'm unsure if there is a Drupal coding standard preference on this.
2. I wonder if the value property is needed if the "mainPropertyName" is now changed to "fetched". Just curious on this one...
3. Should the new TpyedData-related classes live next to the FieldType definition instead of at the src root?

afinnarn → changed the visibility of the branch 3300098-entity-field-fetch to hidden.

I had some questions that I jotted down and tried to answer after a discussion with @swirt.

1. JSON:API schema - With "value" as a boolean is that what the schema output says in Open API docs?

This comes out correctly as "any" in the schema, and the title and description can be updated accordingly.

2. Check the database record for an EFF with the updated change to value. Does all the content get saved?

Changing the property definition for "value" to "any" type and setting its value as the fetched data caused the value to be "1" instead of "0" in my testing. So, it is still saved as a boolean, but I think any data presented is regarded as TRUE.

3. Do any errors appear in logs if you save a node with an EFF on it?

I did not see any errors in the watchdog logs or added to the edit screen.

---

So I think this solution looks promising enough to put up a MR, and I will work on that now. I still have the following questions to answer.

1. GraphQL field - Does that function the same after updating the value property?
2. How to add a test to prove this works? Maybe start with GraphQL and JSON:API tests since that's the way these fields are being used currently in my project.
3. I tried this on a site where the module is already installed. I think a test can prove this works on a new site, but we could also manually test.

This might not be a final solution, but I did a bit of digging on the blog article and connected Drupal issues.

Adding a class to the field property definition, like "->setClass(MyComputedField::class)" in the blog article, didn’t work for me, and I kept getting errors on the code trying to get the property definitions correctly.

Then I saw the note about "->setInternal(FALSE)" and the code around that. The patch in https://www.drupal.org/project/jsonapi/issues/3055163 → got me onto the "$properties['value']" definition in the EntityFieldFetchItem's "propertyDefinitions".

I updated the type to "DataDefinition::create('any')"...cause that's what I do in TypeScript all day ;), but also to test this out without proper typing getting in the way. Then in "setValues" I added "$values['value'] = $values['fetched'];". Loading the JSON:API response gave me the field info I was expecting!

So, I think the fields have "FALSE" in JSON:API responses since the value is set to a boolean and the default is always false. I also tried setting the value property data type to a string with a default of "foo" to see that in the JSON:API response. And the field output was "foo".

I'll leave this here for comment as to whether this direction makes sense to pursue and create an actual patch or MR.

I updated the composer.json file to pull swagger-ui 5.x but didn't add any tests yet. Feel free to review, and hopefully, I can help add tests.

afinnarn → created an issue.

I was trying to add a form just now, but thinking more about it, the "openapi" module should probably include a section in "OpenApiListController" to hold the configuration link to fit with the current UI pattern. I got stuck on what the proper path should be and how to handle config forms within that.

For example, the path "/admin/config/services/openapi/swagger/my_json_api" shows the Swagger UI, but how would you add a setting form to that? "/admin/config/services/openapi/swagger/settings" isn't a good idea, and "/admin/config/services/openapi/settings/swagger" is also weird.

So, I won't try to add the form right now until I look more at the main openapi module. Any thoughts are welcome, but that will be my approach to furthering this ticket.

@drugan I like the additional filter, but I also looked at https://github.com/swagger-api/swagger-ui/blob/master/docs/usage/configu... and added some more options in the patch I uploaded.

What do you think about adding more defaults than filters? And also about adding a configuration form so we can modify config via CLI or UI?

Based on feedback from a reviewer I am putting this back into needs work. There is at least an issue with menu links as you can see in the screenshot below.

...err welp, I thought the image upload would be different. The menu link text is HTML in a "" tag.

It would be good if we can add a test at a higher level that works with a Drupal instance, adds media, and then inserts the media for both content and field parsing.

I've added some PHP Unit tests to cover this code change. I'm used to the old Drupal CI and I have to say the new GitLab instance is much, much better to work with. It even comes with all test running automagically setup with the include of a stock ".gitlab-ci.yml" file. There are warnings about linting errors on the MR, but the unit tests pass. At some point, an issue should be created to take care of the linting errors or change the linting configuration.

On the MR, I think I've done everything necessary and taken care of people already using the module with the update hook. I will try to test the update hook now locally on a Drupal site, but the code is ready to review.

I don't have a preference on what the description text is for the appending pattern form field so I'm open to suggestions on that. I think it is clear enough, but maybe there are Drupal conventions to use for describing tokens like that.

afinnarn → created an issue.

@rjhammond Can you review the duplicate issue, if you have time, so we can call it RTBC?

I tried several different configurations of the module's settings, but I can always edit the media and "?edit-media" query parameter also works. I'm marking this as "needs more info" until someone can provide specific settings that cause this error or more people report similar issues.

Un-assigning myself so someone else can review.

Sounds good to me. I will close this issue as a duplicate.

Hello. I looked at the patch and it has "'system/files" hardcoded as the base path for private files. I made a MR that should cover the private scheme as well as the public scheme.

Please take a look, see if it works for you, and let me know what you think.

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

I errantly created an issue fork here, but I'm still not sure of your issue.

Instead, I created another issue for what I mentioned with content parsing and made a MR here if you want to test: https://www.drupal.org/project/media_link_enhancements/issues/3394047 🐛 Content parsing broken Needs review

I'm used to adding patches, but hopefully, I did the MR workflow correctly.

I also added a .gitignore for IntelliJ files since many Drupal contributors use PHPStorm (including me) and we'd have to ignore those files with every contribution.

afinnarn → created an issue.

Hello @rjhammond,

I'm still getting my bearings on this codebase, but I just tested on 1.0.2, Drupal 10.1.4, and PHP 8.2 where I do see type and size appending working on the link fields but not in the "content parsing" when I tested on a file media item. I set it to "Redirection" for delivery.

However, I figured out the issue with content parsing, or at least what I'm seeing.

In the "MediaLinkEnhancementsAlterLinks" class, there is a method called "alterLinks". In that method around line #151, there is a function call to get the HTML of the full link so it can be checked in replacement further down in the code.

// Get the full link to replace before we modify it.
$full_link = $link->C14N();

If you update that line to:

// Get the full link to replace before we modify it.
$full_link = $doc->saveHTML($link);

The problem was the HTML attributes of the anchor tag were re-arranged by "$link->C14N()". So when the strings were compared in "$content = str_replace($full_link, $new, $content);" the anchor link didn't match exactly and the file extension wasn't added. With "$doc->saveHTML($link)" the attributes are in order so the content matches and is replaced.

I will work on a MR for this, but if you can, please try changing that code to see if content parsing works.

afinnarn → created an issue.

The changes so far look good to me with disabling the button label. Still missing the ability to rename the label text, but maybe that part is not wanted. Let me know if this needs reviewed or any help with the MR.

Yeah, I know the title is used now, but I thought maybe "event.source" had "event.source.html" to match the template. No reason to ever have the exact same template, as was my case. But then again, having templates with the same title is a bad practice.

I think the change to the readme is good enough. Thanks!

afinnarn → created an issue.

afinnarn → created an issue.

Maybe Drupal has to go all in to integrate with Python? So it would be easier to adopt all the richness of Python ecosystem across Drupal websites? (and so, adopt llama index)

I wholeheartedly agree with trying to leverage the Python libraries vs. trying to port them to PHP and keep up with the original Python library. IMHO, that approach would be far easier to maintain as you mention PHP not having as much activity in the AI/ML space as Python does...and PHP never will have a lot of activity since "connector tools" allow you access to the Python libraries without having to port the code to another language.

Another benefit is if there is a list of "data connectors" for ChatGPT/Llama Index that includes Drupal, people can say "hmm, what's this Drupal thing?" and try Drupal out vs. not trying to integrate and "stay on the island" so only Drupal and PHP devs are aware of how AI tools can be integrated and used in a structured CMS system.

My two cents...

The issue with email addresses and anchor links seems to stem from the "URL_REG_EXP" variable in "/src/autoanchor.js" of the zip/library I pulled from #88. It captures links correctly, but I think it should exclude email addresses from auto-anchoring.

I tested by adding this code around line #106 of "/src/autoanchor.js" in the "_enableTypingHandling()" function. I am able to anchor an email manually after updating the code so all it attempts to do is prevent auto-anchoring.

// 3. Check if the URL is an email address.
if (url.match(/[^\s@]+@[^\s@]+\.[^\s@]+/gi)) {
	return;
}

Of course, the regex can be improved and the fix should be updating "URL_REG_EXP" or "getUrlAtTextEnd( text )" in "/src/autoanchor.js" but this is a quick way to test and see if it fixes your issue @LRoels. Plus regex is not my strong suit, and I'm not sure how we're supposed to be working with the JS library. Exchanging zip files seems weird to me vs. hosting it in the module or on npm.

I've tested the patch in #98 with "3.0.x-dev@dev" and I do see the same issue as @LRoels reports about email addresses getting converted to anchor links. I didn't try any other special characters in a string to see if those also turned into anchor links, but I will look at the code further now to see what might be happening.

Okay, the relevant patch in the Acquia Purge module is here and it says to re-roll into this patch: https://www.drupal.org/project/acquia_purge/issues/3157178 →

I will look into this now in my local dev environment.

I'm not sure how helpful this comment is, but I tried the patch in #54 and got the following error.

The website encountered an unexpected error. Please try again later.
ArgumentCountError: Too few arguments to function Drupal\purge\Plugin\Purge\TagsHeader\TagsHeaderBase::__construct(), 3 passed in /var/www/html/docroot/modules/contrib/acquia_purge/src/Plugin/Purge/TagsHeader/AcquiaCloudSiteHeader.php on line 41 and exactly 4 expected in Drupal\purge\Plugin\Purge\TagsHeader\TagsHeaderBase->__construct() (line 32 of modules/contrib/purge/src/Plugin/Purge/TagsHeader/TagsHeaderBase.php). 

I had to switch to other work and didn't have time to look into the error, but I will update the thread if I do. Posting since someone else might encounter the error or know how to fix in a patch.

@himanshu_jhaloya It looks like you have the module disabled when you are analyzing the upgrade status. You need to have the module installed for the scan to properly work. Please try scanning while the module is enabled, which you will have to have a Drupal 9 site to do.

After applying the patch I see "No problems found" in the upgrade status UI and "No known issues found." analyzing via drush. I will set the issue to RTBC.

volkswagenchick → credited afinnarn → .

@Harlor hopefully you get this notification as I think other comments misspelled your handle...would be nice if drupal.org helped us complete usernames, but oh well. This thread has a few missed comments where people were one/two letters off the actual screen name. I will also ping @sonnykt as I think you meant to ask them a question.

But my main question is if your MR is good to be reviewed since you moved the status from "Needs Review" to "Needs Work"?

I'm trying to familiarize myself with the module now, but I can help review it if you are paused. I just don't want to look too hard if you are planning to add updates soon.

I've clearly lost my way and traveled down a wicked path. Unassigning myself to repent.

It's been a while since I've patched or maybe this should be a MR now...but whatever, here is a patch that works for me on at least the alpha2 release of 3.0.0.