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.).

Files: 
CommentFileSizeAuthor
#37 node_field_translation_test-1824244-37.patch1.6 KBcsg
PASSED: [[SimpleTest]]: [MySQL] 48,966 pass(es).
[ View ]
#35 node_field_translation_test-1824244-35.patch1.57 KBcsg
FAILED: [[SimpleTest]]: [MySQL] 48,570 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#29 node_field_translation_test-1824244-29.patch1.6 KBcsg
PASSED: [[SimpleTest]]: [MySQL] 48,847 pass(es).
[ View ]
#27 node_field_translation_test-1824244-27.patch2.15 KBcsg
PASSED: [[SimpleTest]]: [MySQL] 48,823 pass(es).
[ View ]
#19 node_field_translation_test-1824244-19.patch1.21 KBcsg
FAILED: [[SimpleTest]]: [MySQL] 48,771 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 node_field_translation_test-1824244-17.patch2.04 KBcsg
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_field_translation_test-1824244-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 node_field_translation_test-1824244-14.patch1.21 KBcsg
FAILED: [[SimpleTest]]: [MySQL] 48,783 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#10 1824244-10.patch857 bytesswentel
Test request sent.
[ View ]
#4 field_ui_bundle_admin_path-1824244-4.patch5.63 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 48,132 pass(es).
[ View ]
field_ui_bundle_admin_path.patch5.63 KBfubhy
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 ]

Comments

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.

field_ui_bundle_admin_path.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, field_ui_bundle_admin_path.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.63 KB
PASSED: [[SimpleTest]]: [MySQL] 48,132 pass(es).
[ View ]

Rerolled.

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.).

That's because field_ui can be disabled. So I guess that's a valid reason to make it private.

Status:Needs review» Reviewed & tested by the community

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.

Title:Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())Change notice: Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed and pushed to 8.x. Thanks!

This will need a change notice.

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?).

Title:Change notice: Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())
Category:task» bug
Status:Active» Needs review
StatusFileSize
new857 bytes
Test request sent.
[ View ]

This broke the translations tab, patch attached,

Change notice also added at http://drupal.org/node/1848180

Status:Needs review» Reviewed & tested by the community

Title:Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())Tests for: Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())
Category:bug» task
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

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.

Status:Active» Needs review
StatusFileSize
new1.21 KB
FAILED: [[SimpleTest]]: [MySQL] 48,783 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, node_field_translation_test-1824244-14.patch, failed testing.

+++ 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

Status:Needs work» Needs review
StatusFileSize
new2.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_field_translation_test-1824244-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I removed all unnecessary spaces and re-rolled the patch.

Status:Needs review» Needs work

The last submitted patch, node_field_translation_test-1824244-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
FAILED: [[SimpleTest]]: [MySQL] 48,771 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good to me, RTBC when it comes back green.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, node_field_translation_test-1824244-19.patch, failed testing.

The patch should have the fix too not just the test :)

Status:Needs work» Needs review

@Gábor - the fix has been committed in #13 already :)

Since my last patch the fix got in, so I re-test the previous patch.

Status:Needs review» Needs work

The last submitted patch, node_field_translation_test-1824244-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.15 KB
PASSED: [[SimpleTest]]: [MySQL] 48,823 pass(es).
[ View ]

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.

Status:Needs review» Needs work

+++ 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).

Status:Needs work» Needs review
StatusFileSize
new1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 48,847 pass(es).
[ View ]

Thanks for the review! I merged the two functions, changed the variables, and eliminated unnecessary permissions.

+++ 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.

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));

Issue tags:+Needs change record

@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!

Status:Needs review» Reviewed & tested by the community

I tested this patch. It works fine.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Needs change record

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.

Status:Needs work» Needs review
StatusFileSize
new1.57 KB
FAILED: [[SimpleTest]]: [MySQL] 48,570 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

@swentel: Thanks for the review! I removed t() from both asserts, please review again.

The last submitted patch, node_field_translation_test-1824244-35.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 48,966 pass(es).
[ View ]

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!

Status:Needs review» Needs work

The last submitted patch, node_field_translation_test-1824244-37.patch, failed testing.

Status:Needs work» Needs review

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.

Status:Needs review» Reviewed & tested by the community

Alright, let's get this in.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes
Issue tags:-Needs change record