Ensuring that jsonapi_query_builder works seamlessly with Gin version 5 is essential to maintain a coherent user experience, leverage the latest Drupal administrative improvements, and support the Drupal community’s adoption of updated best practices.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

vipin.mittal18 created an issue. See original summary.

vipin.mittal18’s picture

Status: Active » Needs work

vipin.mittal18’s picture

Status: Needs work » Needs review
ankitv18’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new547.22 KB
new418.13 KB

I created a symlink to version 5 of gin at my local to validate it using MR!5 of issue https://www.drupal.org/project/jsonapi_query_builder/issues/3529219.

Adding screenshot for your reference
Install
page

ankitv18’s picture

Assigned: vipin.mittal18 » Unassigned
ankitv18’s picture

balsama’s picture

Status: Reviewed & tested by the community » Needs work
Parent issue: #3529219: Replace JSON:API Schema dependency with Open API JSON:API module »

Thanks for this. Not necessarily related to this MR, but we should look into the following while we're working on this:

  1. We check to see if Gin is installed, but not whether it's the admin theme. Should we check to see if it's actually the admin theme?
  2. What is the user experience if Gin _isn't_ installed? Is it completely broken? If so, we should change the severity level of the requirements check.
  3. Should the requirements check run on install instead of runtime?
vipin.mittal18’s picture

This module also works with another theme without causing UI breaks. Is there any reason to place the Gin theme condition?

traviscarden changed the visibility of the branch 3537554-add-support-for to hidden.

traviscarden changed the visibility of the branch 3537554-add-support-for to active.

traviscarden changed the visibility of the branch 3537554-remove-gin-dependency to hidden.

traviscarden changed the visibility of the branch 3537554-remove-gin-dependency to active.

traviscarden’s picture

Status: Needs work » Needs review

I propose we remove the dependency altogether. I've successfully tested with with Gin 4.x and 5.x, Claro, and even Olivero, and it works fine in all of them. (It's squished in Olivero, of course, but it still works; and if you're using Olivero for your admin you're probably already collecting rent money from the voices in your head.)

In fact, it works so well in Claro, most people would probably be hard-pressed to tell the difference. Since that's the case with the default admin theme in Core, I don't think it's even worth mentioning Gin, much less putting it in hook_requirements().

Assuming we all agree on that, we just need to make one other decision: do we completely remove the Gin requirement from composer.json or not? It would technically be a backward compatibility break, because someone could have the theme installed solely as a transitive Composer dependency (i.e., because they've required JSON:API Query Builder and it in turn requires Gin). If we just remove it from our requirements, it would suddenly disappear from that person's codebase when they update. But I don't know if we have any sort of BC commitment. So...

  1. If we need to preserve backward-compatibility, we should just loosen the Composer requirement so that it also supports Gin 5.x (the existing MR).
  2. If not, we should remove the requirement altogether (a new MR I've opened).

I've removed the hook_requirements() implementation in both MRs, so unless I'm missing something, I think we're ready to just choose between them.

Technical notes:

  1. Tests are going to fail, and that's expected. They're already failing in HEAD, and they're being addressed in #3529219: Replace JSON:API Schema dependency with Open API JSON:API module.
  2. There is a js/react-app/src/gin.css and corresponding HTML classes in the frontend code that seem to imply Gin support, but as far as I can tell, they're not actually specific to Gin (they create no implicit dependency), and they didn't cause any problems in any of the other themes I tested. There's nothing about them in the documentation or the commit log. I'm guessing the naming convention is just an artifact of AI oddities. I suggest we just leave them in there until and unless we a more rigorous audit/clean-up in the future.
  3. I deleted the entire jsonapi_query_builder.install file in both MRs, because all it had was the hook implementation. That will probably cause a merge conflict with #3529219: Replace JSON:API Schema dependency with Open API JSON:API module, but I think there would've been one either way, and it will be trivial to resolve.
balsama’s picture

Version: 1.0.x-dev » 2.0.x-dev

Moved target to 2.x branch

balsama’s picture

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

balsama’s picture

Title: Add support for Drupal Gin latest version 5 » Remove Gin dependency

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.