Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I am not entirely sure why this function is marked private (prefixed with an underscore). It's simply a helper function that can be quite useful in other places than too. Currently we are often hard-coding the paths to the Field UI pages in multiple places because of this function being "private". (e.g. see node_overview_types + node_entity_info, etc.).
Comment | File | Size | Author |
---|---|---|---|
#37 | node_field_translation_test-1824244-37.patch | 1.6 KB | csg |
#35 | node_field_translation_test-1824244-35.patch | 1.57 KB | csg |
#29 | node_field_translation_test-1824244-29.patch | 1.6 KB | csg |
#27 | node_field_translation_test-1824244-27.patch | 2.15 KB | csg |
#19 | node_field_translation_test-1824244-19.patch | 1.21 KB | csg |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedIf there are no objections to do this then I would re-factor the parts in core where we are currently by-passing that function.
Comment #2
swentel CreditAttribution: swentel commentedfield_ui_bundle_admin_path.patch queued for re-testing.
Comment #4
webflo CreditAttribution: webflo commentedRerolled.
Comment #5
swentel CreditAttribution: swentel commentedThat's because field_ui can be disabled. So I guess that's a valid reason to make it private.
Comment #6
yched CreditAttribution: yched commentedWell, it's a function provided by field_ui, so making it public would mean usual caller precautions to check if the module is enabled.
I'd say this makes sense. Other modules might want to link to field UI pages.
Comment #7
Fabianx CreditAttribution: Fabianx commented#4: field_ui_bundle_admin_path-1824244-4.patch queued for re-testing.
Comment #8
webchickCommitted and pushed to 8.x. Thanks!
This will need a change notice.
Comment #9
fubhy CreditAttribution: fubhy commentedThis still needs a follow-up to find the parts in core where we are hard-coding the path to existing manage fields / manage display pages (if there are any?).
Comment #10
swentel CreditAttribution: swentel commentedThis broke the translations tab, patch attached,
Comment #11
swentel CreditAttribution: swentel commentedChange notice also added at http://drupal.org/node/1848180
Comment #12
fubhy CreditAttribution: fubhy commentedComment #13
catchOK this could do with some test coverage, but it suggests we've got no coverage of that page at all at the moment. Moving back to major task to add that. Committed/pushed the fix to 8.x.
Comment #14
csg CreditAttribution: csg commentedI created this patch for #1849316: Undefined function _field_ui_bundle_admin_path when no field is translatable but it seems to be a duplicate of this issue so I upload it here instead. This checks if the Translate page of a translatable node type without any translatable fields is rendered correctly.
If you think more test coverage is needed, please specify, and I'll try to help.
Comment #16
swentel CreditAttribution: swentel commentedSpaces (or tabs, they shouldn't be there).
Same here
Comment #17
csg CreditAttribution: csg commentedI removed all unnecessary spaces and re-rolled the patch.
Comment #19
csg CreditAttribution: csg commentedComment #20
swentel CreditAttribution: swentel commentedLooks good to me, RTBC when it comes back green.
Comment #22
Gábor HojtsyThe patch should have the fix too not just the test :)
Comment #23
csg CreditAttribution: csg commented#19: node_field_translation_test-1824244-19.patch queued for re-testing.
Comment #24
swentel CreditAttribution: swentel commented@Gábor - the fix has been committed in #13 already :)
Comment #25
csg CreditAttribution: csg commentedSince my last patch the fix got in, so I re-test the previous patch.
Comment #27
csg CreditAttribution: csg commentedThe test failed because it requires the field_ui module and it wasn't enabled. I fixed that, and also added another test that checks if the Translate page of a translatable node type with translatable fields is rendered correctly.
Comment #28
Gábor HojtsyI don't think these two need to be separate functions. You can do an assert before the field is removed and an assert after?
These two can/should be local variables since the tests don't have global variables for this I believe.
Also, the permissions granted are too wide for this task. Sounds like 'translate any entity' is required to access this node translation page and 'access administration pages' was needed for the field UI? It would be great to test with just the permissions needed.
For that, the test class has a translator user which you can maybe use (see other tests).
Comment #29
csg CreditAttribution: csg commentedThanks for the review! I merged the two functions, changed the variables, and eliminated unnecessary permissions.
Comment #30
XanoFor readability, it is recommended to put every item of an associative array on its own line.
// Edit: I know it's a widely used pattern, but it's still a lot less readable than putting items on separate lines.
Comment #31
Gábor HojtsyXano: this is a very widely used pattern when only a couple arguments are specified. Some examples:
Comment #32
csg CreditAttribution: csg commented@Xano: Every other function of this class uses the same pattern, so I think it's more consistent this way. Are you sure it would be feasible to change it?
Thanks to both of you for the review!
Comment #33
mr.york CreditAttribution: mr.york commentedI tested this patch. It works fine.
Comment #34
swentel CreditAttribution: swentel commentedExtreme nitpick, but it's a new guideline for tests.
We don't use t() anymore in asserts. Either simply use quotes and if you need to replace variables, use format_string()
Same here, otherwise looks good though.
Comment #35
csg CreditAttribution: csg commented@swentel: Thanks for the review! I removed t() from both asserts, please review again.
Comment #37
csg CreditAttribution: csg commentedErm, I had been tinkering a little more with the permissions to see if I can narrow it down any further, and forgot to change it back... Now it's fixed.
Please review it once again!
Comment #39
csg CreditAttribution: csg commented#37: node_field_translation_test-1824244-37.patch queued for re-testing.
Comment #40
csg CreditAttribution: csg commentedPrevious result: FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
Since it has nothing to do with my patch, I queued it for re-testing.
Comment #41
swentel CreditAttribution: swentel commentedAlright, let's get this in.
Comment #42
webchickCommitted and pushed to 8.x. Thanks!
Comment #44
xjm