Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep only important methods in ContentPartDisplayDriver and ContentFieldDisplayDriver #16530

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Aug 6, 2024

The ContentPartDisplayDriver base class have undergone modifications. The following methods were marked obsolete:

  • IDisplayResult Display(TPart part)
  • IDisplayResult Edit(TPart part)
  • Task<IDisplayResult> UpdateAsync(TPart part, IUpdateModel updater)

At the same time, the following method was removed:

  • Task<IDisplayResult> UpdateAsync(TPart part, IUpdateModel updater)

Moreover, the ContentFieldDisplayDriver base class have undergone modifications. The following methods were marked obsolete:

  • UpdateAsync(TField field, IUpdateModel updater, UpdateFieldEditorContext context)
  • Update(TField field, IUpdateModel updater, UpdateFieldEditorContext context)

At the same time, the following method was added:

  • UpdateAsync(TField field, UpdateFieldEditorContext context)

@MikeAlhayek MikeAlhayek changed the title Keep only important methods in ContentPartDisplayDriver and ContentFi… Keep only important methods in ContentPartDisplayDriver and ContentFieldDisplayDriver Aug 6, 2024
@MikeAlhayek MikeAlhayek requested a review from Piedone August 6, 2024 21:11
@Piedone
Copy link
Member

Piedone commented Aug 7, 2024

I'll be a buzzkill, and I'm sorry about that, but I'm starting to doubt if continuing these driver updates is productive. Sure, it's nice to have and I enjoy code clean-ups, but it's emphatically nice, not necessary, and every single consumer out there will need to be adapted. Can you please convince me this is feasible?

Changes like this also delay the completion of the work @sarahelsaig is doing on upgrading three huge solutions by days (until the preview package is ready, she gets back to the work, somebody else reviews...). That'll never be done if we keep adding breaking changes (a change that causes a warning is also breaking for any respectable codebase). At one point, we have to stop with that, leave 2-3 weeks for Sári and the rest of the community living on the edge to catch up and report any regressions, and only then can we release 2.0.

@MikeAlhayek
Copy link
Member Author

I understand. But I feel these changes go hand-on-hand with the other PR for consistency. By the way, I noticed these drives as I was upgrading my projects to the latest driver changes.

I don't think it's much work to upgrade this. As you saw I submitted a PR for the commerce module, that did not take me much time at all. Plus people that upgrade to 2.0.0, will address all the warnings and build errors at the same time "unlike the extra steps that @sarahelsaig is having to do now).

I suggest we take these changes very soon so anyone is updating to the latest previews can make all the required changes at once.

Copy link
Contributor

github-actions bot commented Aug 7, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

@Piedone
Copy link
Member

Piedone commented Aug 7, 2024

OK, let's get over with it at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants