Posted by fubhy on October 26, 2012 at 4:25pm
15 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.).
Change records for this issue
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| field_ui_bundle_admin_path.patch | 5.63 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_ui_bundle_admin_path.patch. Unable to apply patch. See the log in the details link for more information. | View details |
Comments
#1
If there are no objections to do this then I would re-factor the parts in core where we are currently by-passing that function.
#2
field_ui_bundle_admin_path.patch queued for re-testing.
#3
The last submitted patch, field_ui_bundle_admin_path.patch, failed testing.
#4
Rerolled.
#5
That's because field_ui can be disabled. So I guess that's a valid reason to make it private.
#6
Well, 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.
#7
#4: field_ui_bundle_admin_path-1824244-4.patch queued for re-testing.
#8
Committed and pushed to 8.x. Thanks!
This will need a change notice.
#9
This 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?).
#10
This broke the translations tab, patch attached,
#11
Change notice also added at http://drupal.org/node/1848180
#12
#13
OK 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.
#14
I 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.
#15
The last submitted patch, node_field_translation_test-1824244-14.patch, failed testing.
#16
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined@@ -60,6 +60,23 @@ function getTranslatorPermissions() {
+ ¶
Spaces (or tabs, they shouldn't be there).
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined@@ -60,6 +60,23 @@ function getTranslatorPermissions() {
+ ¶
Same here
#17
I removed all unnecessary spaces and re-rolled the patch.
#18
The last submitted patch, node_field_translation_test-1824244-17.patch, failed testing.
#19
#20
Looks good to me, RTBC when it comes back green.
#21
The last submitted patch, node_field_translation_test-1824244-19.patch, failed testing.
#22
The patch should have the fix too not just the test :)
#23
#19: node_field_translation_test-1824244-19.patch queued for re-testing.
#24
@Gábor - the fix has been committed in #13 already :)
#25
Since my last patch the fix got in, so I re-test the previous patch.
#26
The last submitted patch, node_field_translation_test-1824244-19.patch, failed testing.
#27
The 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.
#28
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined@@ -60,6 +60,37 @@ function getTranslatorPermissions() {
/**
+ * Tests field translation form when fields are translatable.
+ */
+ function testTranslationWithTranslatableFields() {
I don't think these two need to be separate functions. You can do an assert before the field is removed and an assert after?
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined@@ -60,6 +60,37 @@ function getTranslatorPermissions() {
+ $this->admin_user = $this->drupalCreateUser(array('administer content types', 'translate any entity', 'access administration pages', 'access content overview', 'administer nodes', 'bypass node access'));
+ $this->drupalLogin($this->admin_user);
...
+ $this->admin_user = $this->drupalCreateUser(array('administer content types', 'translate any entity', 'access administration pages', 'access content overview', 'administer nodes', 'bypass node access'));
+ $this->drupalLogin($this->admin_user);
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).
#29
Thanks for the review! I merged the two functions, changed the variables, and eliminated unnecessary permissions.
#30
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php@@ -77,6 +77,27 @@ function testTranslateLinkContentAdminPage() {
+ $article = $this->drupalCreateNode(array('type' => 'article', 'langcode' => 'en'));
For 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.
#31
Xano: this is a very widely used pattern when only a couple arguments are specified. Some examples:
core/modules/node/lib/Drupal/node/Tests/NodeAccessRecordsTest.php: $node2 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 0));core/modules/node/lib/Drupal/node/Tests/NodeAccessRecordsTest.php: $node3 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 0, 'status' => 0));
core/modules/node/lib/Drupal/node/Tests/NodeAccessRecordsTest.php: $node4 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 1));
core/modules/node/lib/Drupal/node/Tests/NodeAccessRecordsTest.php: $node6 = $this->drupalCreateNode(array('status' => 0, 'disable_node_access' => TRUE));
core/modules/node/lib/Drupal/node/Tests/NodeLoadHooksTest.php: $node1 = $this->drupalCreateNode(array('type' => 'article', 'status' => NODE_PUBLISHED));
core/modules/node/lib/Drupal/node/Tests/NodeLoadHooksTest.php: $node2 = $this->drupalCreateNode(array('type' => 'article', 'status' => NODE_PUBLISHED));
core/modules/node/lib/Drupal/node/Tests/NodeLoadHooksTest.php: $node3 = $this->drupalCreateNode(array('type' => 'article', 'status' => NODE_NOT_PUBLISHED));
core/modules/node/lib/Drupal/node/Tests/NodeLoadHooksTest.php: $node4 = $this->drupalCreateNode(array('type' => 'page', 'status' => NODE_NOT_PUBLISHED));
core/modules/node/lib/Drupal/node/Tests/NodeLoadMultipleTest.php: $node1 = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1));
core/modules/node/lib/Drupal/node/Tests/NodeLoadMultipleTest.php: $node2 = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1));
core/modules/node/lib/Drupal/node/Tests/NodeLoadMultipleTest.php: $node3 = $this->drupalCreateNode(array('type' => 'article', 'promote' => 0));
core/modules/node/lib/Drupal/node/Tests/NodeLoadMultipleTest.php: $node4 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 0));
#32
@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!
#33
I tested this patch. It works fine.
#34
Extreme nitpick, but it's a new guideline for tests.
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined@@ -77,6 +77,27 @@ function testTranslateLinkContentAdminPage() {
+ $this->assertRaw(t('Not translated'));
We don't use t() anymore in asserts. Either simply use quotes and if you need to replace variables, use format_string()
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined@@ -77,6 +77,27 @@ function testTranslateLinkContentAdminPage() {
+ $this->assertRaw(t('no translatable fields'));
Same here, otherwise looks good though.
#35
@swentel: Thanks for the review! I removed t() from both asserts, please review again.
#36
The last submitted patch, node_field_translation_test-1824244-35.patch, failed testing.
#37
Erm, 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!
#38
The last submitted patch, node_field_translation_test-1824244-37.patch, failed testing.
#39
#37: node_field_translation_test-1824244-37.patch queued for re-testing.
#40
Previous 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.
#41
Alright, let's get this in.
#42
Committed and pushed to 8.x. Thanks!
#43
Automatically closed -- issue fixed for 2 weeks with no activity.