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
[D8] Port and merge Field Formatter Settings in core #1376
Comments
Thanx @laryn 😉 |
@klonos FYI: https://github.com/backdrop-contrib/field_formatter_settings Barely needed to be touched from what I could tell, it works for me (at least as far as Field Multiple Limit is concerned, which listed it as a dependency). |
RAW Pull-Request backdrop/backdrop#2493 late night testing with https://github.com/backdrop-contrib/field_multiple_limit, and it seems to work ! |
Adding documentation from field_formatter_settings to our |
Is it ready to be reviewed and what would be the best way? |
@herbdool I have added some additional info in the issue summary in the hopes that that would make it easier to understand how the contrib module worked. I guess best way to test is to try to install a contrib module such as https://github.com/backdrop-contrib/field_multiple_limit or https://github.com/backdrop-contrib/field_html_trim that have https://github.com/backdrop-contrib/field_formatter_settings as a dependency, but without installing Another thing that would help is the code example in the D8 change record: https://www.drupal.org/node/2130757 |
...so:
|
I have tested this with https://github.com/backdrop-contrib/field_multiple_limit and it works as expected, so RTBC. Adding milestone candidate for 1.13 PS: Noting that the Views UI settings for |
Thanks @klonos for all these useful comments <3 |
Any thought for adding a milestone here ? @jenlampton @quicksketch @klonos |
Yes, I agree with the milestone candidate label. Adding to 1.13 :) |
I hate to be the bad guy again here, but we need test coverage in order to add this feature. Anything that adds functionality that is not directly used by core needs a test module that uses that functionality, then tests that ensure it works. Otherwise we'd have no way of knowing if we broke functionality of something provided but not utilized by core. |
The Drupal patch seems to include tests which probably work here without too much trouble. https://www.drupal.org/project/drupal/issues/945524#comment-6843398 |
but you're right. I need to learn how to write nice tests for Backdrop! |
@opi I tried out this patch for tests https://www.drupal.org/files/945524-55-tests_only.patch. It mostly works except one failure which I can't figure out. I might push it to your PR and can get more eyes on it. |
@opi tests are passing now. |
I've merged backdrop/backdrop#2493 into 1.x for 1.13.0. Thanks @opi, @laryn, @klonos, and @herbdool! We need a follow-up here though to add |
Follow-up issue: #3742 |
Another module that's merged in D8 core: https://www.drupal.org/project/field_formatter_settings
Part of #378
D8 change record: https://www.drupal.org/node/2130757
@mattfarina https://www.drupal.org/project/drupal/issues/945524#comment-3591916
Modules that require the Field formatter settings module:
Current usage count at the time of writing (Feb, 2019): ~52k
PR by @opi: backdrop/backdrop#2493
The text was updated successfully, but these errors were encountered: