Tests for: Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())

field_ui_bundle_admin_path.patch queued for re-testing.

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.

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.

#4: field_ui_bundle_admin_path-1824244-4.patch queued for re-testing.

Committed and pushed to 8.x. Thanks!

This will need a change notice.

This broke the translations tab, patch attached,

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

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.

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.

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

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

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

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

#19: node_field_translation_test-1824244-19.patch queued for re-testing.

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

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.

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

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

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

I tested this patch. It works fine.

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.

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

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!

#37: node_field_translation_test-1824244-37.patch queued for re-testing.

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.

Alright, let's get this in.

Committed and pushed to 8.x. Thanks!