Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Closed (won't fix)

Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.

ayelet_Cr’s picture

Status: Closed (won't fix) » Active
robeano’s picture

Assigned: Unassigned » robeano

I'll take entity_test module starting with this issue.

robeano’s picture

Assigned: robeano » Unassigned

I will likely run out of time to work on this patch. Unassigning myself.

disasm’s picture

Assigned: Unassigned » disasm
disasm’s picture

Title: Convert entity_test_add() to a new style controller » Convert entity_test callbacks to a new style controller
Status: Active » Needs review
FileSize
6.84 KB

first pass.

Status: Needs review » Needs work

The last submitted patch, drupal8.entity_test.1987694-6.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
6.85 KB

Changing _access to _permission since it's not a callback.

Status: Needs review » Needs work

The last submitted patch, drupal8.entity_test.1987694-8.patch, failed testing.

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

vijaycs85’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.32 KB

Re-roll+ drupal_set_title() update...

Status: Needs review » Needs work

The last submitted patch, 11: 1987888-entity_test-controller-11.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: 1987888-entity_test-controller-11.patch, failed testing.

ianthomas_uk’s picture

Assigned: disasm » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +@deprecated
FileSize
3.6 KB

Re-roll, resolved conflict in entity_test.module

Status: Needs review » Needs work

The last submitted patch, 15: 1987888-entity_test-controller-15.patch, failed testing.

The last submitted patch, 15: 1987888-entity_test-controller-15.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
ianthomas_uk’s picture

@Wim Leers have you been looking into this or did you just want fresh test results before you started? I've been having a look this evening, I spotted that the new code is calling entityManager() when I think it should be entityFormBuilder() and was about to test a patch to change that, but can leave you to it if you're already working on it.

Status: Needs review » Needs work

The last submitted patch, 15: 1987888-entity_test-controller-15.patch, failed testing.

ianthomas_uk’s picture

FileSize
3.61 KB

As mentioned, entityManager() > entityFormBuilder(). Fixes the tests I've tried locally.

ianthomas_uk’s picture

Status: Needs work » Needs review
ianthomas_uk’s picture

Priority: Normal » Major
rlmumford’s picture

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Controller/EntityTestController.php
@@ -7,27 +7,48 @@
+   * @see entity_test_menu()

Does entity_test_menu() still exist?

Apart from that question this all looks good to me. Happy to RTBC.

rlmumford’s picture

Status: Needs review » Needs work

Yeah, entity_test_menu() was removed in 8608fed

We should probably update the docblock.

rlmumford’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
1.05 KB

Here's an updated patch.

ianthomas_uk’s picture

FileSize
3.84 KB

That was moved to \Drupal\entity_test\Routing\EntityTestRoutes::routes(), this patch puts an appropriate @see on each method, rather than just removing it.

rlmumford’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

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.