search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Added: entity reference module in core. #1301

Closed jenlampton closed 2 years ago

jenlampton commented 9 years ago

Description of the need

I use node and user references all the time. It would be great if we could add these to core.

Proposed solution (Updated 2022)

Move the Entity Reference contrib module into core. It has been refactored to remove any external dependencies. Entity Reference is the most obvious candidate based on the following advantages:

1) It has the highest install count. 2) It has the most flexible API and number of extending modules. 3) It is a proven solution and includes an upgrade path for D7 Entity Reference users.

Previous solution (Prior to 2022)

This was the original solution proposed in 2015. After the stalling of the new "Reference" project and the subsequent porting and updating of Entity Reference, this approach has failed to gain traction.

1) We should create a new module reference that is a rewrite of the Drupal 7 entityreference module, in language that is simpler and easier to understand. It will more closely resemble node_reference and user_reference from the Drupal 7 references module. There is now a new Reference module in contrib 2) Once the module is ready, we should move this module into core. When is the module "ready"? When it has support for node references, and all blockers in the contrib queue have been resolved.

Core Blockers for adding the reference module into core

We also have a single issue in the module queue documenting all the issues there that need to be resolved for core inclusion: META: Blockers for getting into core

Alternatives that have been considered

Additional information

Follow-up for merging in some bug fixes after this is merged:

We have a follow-up issue for removing the term-reference field, and replacing it with a reference field. We'll get support for terms in first, long before we remove the previous term reference field:

It's been recommended that we mark that field type as deprecated for 1 major cycle of backdrop core, and remove it in the next. Use this issue for moving term reference field out of taxonomy module and marking it as deprecated:

The core issue below is needed for file entity support:

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes a new field type that can be used to reference other entities such as content, categories, or user accounts.

Advocate: @herbdool

quicksketch commented 2 years ago

There's actually a whole issue about this question of replacing Term Reference at https://github.com/backdrop/backdrop-issues/issues/51.

quicksketch commented 2 years ago

I think the only big hold-up here is gracefully handling both the contrib and core module at the same time. Since we've done this before with other modules (like Link, Email, Redirect) we can repeat the same approach.

I pushed https://github.com/backdrop/backdrop/pull/4033/commits/0d907a6d6dfb8bd9dd735424801098ae4370c37f to the PR at https://github.com/backdrop/backdrop/pull/4033 that links to a draft change notice at https://docs.backdropcms.org/node/47321 (unpublished).

I tested downloading the contrib module, installing it, then switching to this branch. The update hook and status report both discovered the contrib version and linked to the change notice.

I did additional testing, tried out different widgets, reference methods, sorting, display modes... everything works great. There are some features I think are a little excessive (such as sorting on any arbitrary column from the base entity table), but if we maintain compatibility with the contrib version we need to continue supporting such options.

There is also test coverage and all tests are passing. Great job @herbdool!

This could use further review, but my impression is we are quite close with this approach.

quicksketch commented 2 years ago

I updated the summary with a more accurate representation of the current approach.

@herbdool Would you be willing to advocate for this issue so we can move it into the 1.23.0 milestone?

herbdool commented 2 years ago

@quicksketch thanks! I can advocate for this.

Are you thinking merging this is number 1 on your list and then number 2 through 4 on your list would be on the other issue #51?

quicksketch commented 2 years ago

Are you thinking merging this is number 1 on your list and then number 2 through 4 on your list would be on the other issue https://github.com/backdrop/backdrop-issues/issues/51?

@herbdool Yes. Moving an entire module history into core like this is too unwieldy to try and wrap more into a single issue.

herbdool commented 2 years ago

Here's a follow-up to separate out the term reference field from the taxonomy module: https://github.com/backdrop/backdrop-issues/issues/5633. I think we'll also need an issue for porting any missing features over to reference, such as the autocreation of tags.

indigoxela commented 2 years ago

There's currently a failing test and it's not a random one. I verified by also running that test in the sandbox.

Entity Reference: Entity Reference Handlers (EntityReferenceHandlersTestCase)

So I'm setting the issue back to "needs work".

indigoxela commented 2 years ago

I played with the PR sandbox - this all looks great! ... Until one tries to create a view of type "Entity reference" on a content type with an entity reference to a term ("category") and tries to filter the view by that category (term name) based on a relationship. View ui does odd things then (input field disappears). And the result stays empty.

Filtering by tid (numeric field value) works like a charm.

My description may be a bit confusing, but using views results for reference fields isn't that uncommon.

The views thing may be handled in a follow-up, if people agree. The broken functional test should get fixed, though, before merging.

(More than 4500 lines of code added - this will be hard to review. :wink:)

argiepiano commented 2 years ago

It'll be great to have this in core! Thanks for the work.

There are 3 PRs in the Entity Reference queue (2 of which I created in the last few days) which are in my view important fixes for issues related to third modules creating behavior and selection handlers for Entity Reference. The changes are minimal, but are important and I'm hoping they will make it to this PR before merging into core

herbdool commented 2 years ago

I've merged in the PRs @argiepiano mentions -- into the contrib module. Now I have to remember how I set up the git repo to merge it into core and update the history here too.

quicksketch commented 2 years ago

@herbdool If it would make things easier, I'm open to merging the current PR without the 3 additional commits. Then we can apply each one of the commits to the core repository directly. Would that work as an approach?

herbdool commented 2 years ago

@quicksketch yes, I think that sounds easier. Let's do it! Can make a follow-up for those commits.

Update: here's the follow-up issue https://github.com/backdrop/backdrop-issues/issues/5665

quicksketch commented 2 years ago

I'll request any concerns from the leadership team (PMC + core committers) via Zulip and here. Any last concerns please speak now.

indigoxela commented 2 years ago

Plus one for merging these 4,508 lines of code and in follow-ups iterate over all quirks, bugs, oddities found while testing. Doing it step by step seems easier for all people involved.

But I do have one request: EntityReferenceHandlersTestCase (Tests for the base handlers provided by Entity Reference.) is still failing. IMO it's better to fix the test before merging. (In other words, let's not make a precedent for merging with failing tests :wink:)

herbdool commented 2 years ago

When I run the tests locally, they all pass. So this is odd and hard to debug. I can try restart the tests here just to see.

indigoxela commented 2 years ago

When I run the tests locally, they all pass.

Very strange. Is your local install "vanilla", or are modules installed that aren't there in the sandbox (where the test also fails)?

The first thing to try may be a rebase (just in case...).

argiepiano commented 2 years ago

I've been able to reproduce the test failure in my local, and will take a closer look today [EDIT: I'm working with a patched current dev version of Backdrop, so the rebase may be needed in the PR]

herbdool commented 2 years ago

It's the new card content. I've updated the test to remove all the default nodes (before it was removing the original 2 nodes, now 5).

argiepiano commented 2 years ago

That's great. Isn't it easier to use the testing profile? That skips creating the default nodes. EDIT: basically declaring 

protected $profile = 'testing';

at the beginning of the test class

herbdool commented 2 years ago

Usually yes. In this case I didn't want to change the old tests too much.

Locally I was able to recreate the failures and failures are gone when I delete the nodes. But tests on github are still failing.

argiepiano commented 2 years ago

Something went wrong with the rebase - there are more failures now than before. Are you rebasing from the current 1.x version?

EDIT: This message makes me suspicious:

Screen Shot 2022-07-04 at 9 37 43 AM

It looks like you merged from your repo branch issue-1301 instead of branch backdrop/1.x, unless I'm misunderstanding the msg.

herbdool commented 2 years ago

I was pulling from the branch that created the PR to my local - wasn't sure if my local was up to date. Didn't want to create a merge and then the fix carried that back up. Anyway. I don't want to rebase since that gets messy quick in this case. So I've done a merge and we'll see what that looks like.

I could always restart with a fresh commit and add back in any changes if this looks messy.

herbdool commented 2 years ago

All tests passing except one unrelated one in BootstrapPageCacheTestCase.

stpaultim commented 2 years ago

It looks to me like we have a version right now with all tests passing! Whoopeee....

quicksketch commented 2 years ago

Merged https://github.com/backdrop/backdrop/pull/4033 in 1.x for 1.23.0. Wow! Thanks @herbdool for putting together this final PR, advocating for Entity Reference as the solution, and your continuous work on Entity Reference in contrib leading up to this!

73c65a0 by @herbdool, @argiepiano, @klonos, @indigoxela, @jenlampton, @docwilmot, @stpaultim, @philsward, @quicksketch, @mikemccaffrey, @BWPanda, @laryn, @hosef, @serundeputy, @olafgrabienski, @MrHaroldA, @keiserjb, and @jromine.

I hope I got everyone that contributed to this issue, even if was in the decision-making process or experimentation with our other options (thanks @mikemccaffrey and @jromine in particular).

herbdool commented 2 years ago

Woohoo!

I made PRs for the follow-up bug fixes in https://github.com/backdrop/backdrop-issues/issues/5665

herbdool commented 2 years ago

I also created a PR for https://github.com/backdrop/backdrop-issues/issues/2300, fixing the File::uri(). Now that entityreference is in core, I changed the other issue to a bug.