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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

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.

swentel’s picture

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.

webflo’s picture

Status: Needs work » Needs review
FileSize
5.63 KB

Rerolled.

swentel’s picture

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.

yched’s picture

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.

Fabianx’s picture

webchick’s picture

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.

fubhy’s picture

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

swentel’s picture

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
FileSize
857 bytes

This broke the translations tab, patch attached,

swentel’s picture

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

fubhy’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

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.

csg’s picture

Status: Active » Needs review
FileSize
1.21 KB

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.

swentel’s picture

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

csg’s picture

Status: Needs work » Needs review
FileSize
2.04 KB

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.

csg’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
swentel’s picture

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.

Gábor Hojtsy’s picture

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

csg’s picture

Status: Needs work » Needs review
swentel’s picture

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

csg’s picture

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.

csg’s picture

Status: Needs work » Needs review
FileSize
2.15 KB

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.

Gábor Hojtsy’s picture

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

csg’s picture

Status: Needs work » Needs review
FileSize
1.6 KB

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

Xano’s picture

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

Gábor Hojtsy’s picture

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));
csg’s picture

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!

mr.york’s picture

Status: Needs review » Reviewed & tested by the community

I tested this patch. It works fine.

swentel’s picture

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.

csg’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

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

csg’s picture

Status: Needs work » Needs review
FileSize
1.6 KB

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.

csg’s picture

Status: Needs work » Needs review
csg’s picture

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.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Alright, let's get this in.

webchick’s picture

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.

xjm’s picture

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