Problem

The configuration translation user interface module needs field operations to be alterable, so we can add a translation operation on field configuration screens. Altering operations is possible for other entity listings, fields are not implemented as entity list controllers.

Proposal

Make operations altering possible. We don't need to convert this to a full on entity list controller to do it, we just need to let the operations be altered.

Files: 
CommentFileSizeAuthor
#31 interdiff-26-31.txt2.8 KBbechtold
#31 2045043-filter-alter-31.patch2.52 KBbechtold
PASSED: [[SimpleTest]]: [MySQL] 57,552 pass(es). View
#26 2045043-filter-alter-26.patch2.04 KBbechtold
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php. View
#23 interdiff-20-23.txt568 bytesbechtold
#23 2045043-filter_alter-23.patch568 bytesbechtold
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2045043-filter_alter-23.patch. Unable to apply patch. See the log in the details link for more information. View
#22 2045043-filter_alter-22.patch5.45 KBbechtold
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php. View
#20 2045043-filter_alter-20.patch1.99 KBbechtold
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php. View
#17 2045043-filter-alter-17.patch1.96 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,325 pass(es). View
#17 interdiff-12-17.txt656 bytesYesCT
#15 field-nopatch.png164.18 KBYesCT
#15 fields-with.png205.53 KBYesCT
#13 2045043-filter-alter-12.patch1.96 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,927 pass(es). View
#13 interdiff-6-12.txt1.16 KBYesCT
#6 2045043-filter-alter-6.patch1.88 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,207 pass(es). View
#3 2045043-filter-alter-3.patch1.84 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,488 pass(es), 20 fail(s), and 106 exception(s). View
#3 2045043-diff-1-3.txt1.87 KBvijaycs85
#1 fields-alter.patch778 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 57,172 pass(es). View

Comments

Gábor Hojtsy’s picture

FileSize
778 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,172 pass(es). View

Initial patch. This would need the module handler injected I think, definitely not just call out to \Drupal simply like it does now :D However this one works. Can someone help with doing the injection of the module handler?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
vijaycs85’s picture

FileSize
1.87 KB
1.84 KB
FAILED: [[SimpleTest]]: [MySQL] 57,488 pass(es), 20 fail(s), and 106 exception(s). View

trying to inject module handler...

Status: Needs review » Needs work

The last submitted patch, 2045043-filter-alter-3.patch, failed testing.

Gábor Hojtsy’s picture

What do we need the entity manager for?

The error reported by testbot is: Argument 1 passed to Drupal\field_ui\FieldOverview::__construct() must be an instance of Drupal\field_ui\EntityManager, instance of Drupal\Core\Entity\EntityManager given. There should be a "use" for Drupal\Core\Entity\EntityManager.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,207 pass(es). View

Seems EntityManager is used in submit.

vijaycs85’s picture

Issue tags: +LONDON_2013_JULY

Updating tag.

amateescu’s picture

Issue tags: -LONDON_2013_JULY

The thing is.. we do want to turn that screen into an EntityListController, and that should happen in #1963340: Change field UI so that adding a field is a separate task.

amateescu’s picture

Issue tags: +LONDON_2013_JULY

Sorry for eating the tag.

vijaycs85’s picture

Updating sprint tag - LONDON_2013_JULY

Gábor Hojtsy’s picture

From the point of view of field config translation, we want to wire it up to this UI but we cannot do it ATM. Unless lack of field config translatability is at most a normal bug, we cannot get the module into core without this present.

yched’s picture

Getting this in separately, and then adjust the code if/when the "manage fields" is turned to a ListController, sounds fine by me ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -10,12 +10,44 @@
+  public function __construct(EntityManager $entity_manager, ModuleHandlerInterface $module_handler) {
+    $this->entityManager = $entity_manager;
+    $this->moduleHandler = $module_handler;
+  }

Why are we also adding an entity manager ? Doesn't look like it's used anywhere (nor documented) ?

YesCT’s picture

FileSize
1.16 KB
1.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,927 pass(es). View

I looked this over. I thought we should have it ready, in case we decide to go ahead with it.
Only thing I could find was missing descriptions for @params
and I put the new use statements as close to alphabetical as I could, only changing the lines already changed.

Everything else looks fine. I think I might be starting to understand all the things to check with injection in terms of the __contstruct, create, having the parameters match and the order of things set in the static.

the entitymanager property comes from OverviewBase

--

I didn't try this with config_translation yet.

YesCT’s picture

@yched

I asked myself the same question about the entitymanager.

we are not using it in the new code in this patch, but it was already used elsewhere.
For example, in the submitForm()

      // Create the field and instance.
      try {
        $this->entityManager->getStorageController('field_entity')->create($field)->save();
        $new_instance = $this->entityManager->getStorageController('field_instance')->create($instance);
        $new_instance->save();

And that was working before this patch added it, because it was inheriting the property and the __construct and create from OverviewBase since
class FieldOverview extends OverviewBase

(At least that is what I think... let me know if otherwise.)

YesCT’s picture

FileSize
205.53 KB
164.18 KB

tested it with config_translation without and with the patch. :)

field-nopatch.png

fields-with.png

yched’s picture

Ah, right, entityManager is defined and documented in the parent class.

No biggie, but I think then it would be slightly more correct if FieldOverview::__construct() did:

  public function __construct(EntityManager $entity_manager, ModuleHandlerInterface $module_handler) {
    parent::__construct(EntityManager $entity_manager);
    $this->moduleHandler = $module_handler;
  }

Other than that, looks good to me :-)

YesCT’s picture

FileSize
656 bytes
1.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,325 pass(es). View

@yched That makes sense, then if the construct there changes, we get those changes here automatically.
My syntax was a bit different, but did #16.

Hopefully this will be ready now.

yched’s picture

This looks good if it comes back green.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

green. cool.

bechtold’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.99 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php. View

rerolled because @YesCT suggested to do it :-)

There was a Conflict in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php.
So i added
use Drupal\Core\Entity\Field\FieldTypePluginManager;

Status: Needs review » Needs work

The last submitted patch, 2045043-filter_alter-20.patch, failed testing.

bechtold’s picture

FileSize
5.45 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php. View

another try, couldn't find a php synax error, but fixed the formatting a bit.

bechtold’s picture

FileSize
568 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2045043-filter_alter-23.patch. Unable to apply patch. See the log in the details link for more information. View
568 bytes

in #22 too much formatting was done.

bechtold’s picture

Status: Needs work » Needs review

in #22 too much formatting was done.

Status: Needs review » Needs work

The last submitted patch, 2045043-filter_alter-23.patch, failed testing.

bechtold’s picture

FileSize
2.04 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php. View

uploaded wrong patch in #23

bechtold’s picture

Status: Needs work » Needs review

needs review

Status: Needs review » Needs work

The last submitted patch, 2045043-filter-alter-26.patch, failed testing.

yched’s picture

The patch tries to add a second __construct() method while there is already one :-)

YesCT’s picture

@bechtold is here at the camp, so I will see if they can do it tonight.

bechtold’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,552 pass(es). View
2.8 KB

#2047993-23: Remove current uses of field_info_*_types() / field_info_*_settings() added the same constructor and create method as our rerolled patch #26.

We fixed it.

Status: Needs review » Needs work

The last submitted patch, 2045043-filter-alter-31.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

FAILED: [[SimpleTest]]: [MySQL] 57,920 pass(es), 2 fail(s), and 0 exception(s).

HTTP response expected 204, actual 400 Browser NodeTest.php 70 Drupal\rest\Tests\NodeTest->testNodes()
Value '0\'v;Vlbc' is equal to value '|~-iJ\\1`'. Other NodeTest.php 74 Drupal\rest\Tests\NodeTest->testNodes()

I think that is a random failure. (#2056713: Random test failure in Drupal\rest\Tests\NodeTest)

YesCT’s picture

#31: 2045043-filter-alter-31.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2045043-filter-alter-31.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-config, +LONDON_2013_JULY

#31: 2045043-filter-alter-31.patch queued for re-testing.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Right, I forgot that #2047993: Remove current uses of field_info_*_types() / field_info_*_settings() would conflict. Thanks for the detective work!

YesCT’s picture

while working on this testing I noticed a different problem.
#2057227: Field instance needs uri() method different from the default

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The new hook needs an example in field.api.php

Also we really ought to have a test implementation - ie. a test module in the field/tests/modules that provides an implementation that can be tested by a test in the field module

alexpott’s picture

Issue tags: +Needs tests
amateescu’s picture

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

The new hook needs an example in field.api.php

Also we really ought to have a test implementation - ie. a test module in the field/tests/modules that provides an implementation that can be tested by a test in the field module

Not really..

So, the patch is a bit weird because it invokes an already existing hook (!) that is usually invoked by the EntityListController, but since the field list is not yet a list controller, Gabor wants to have this "shortcut" for the config translation module until #1963340: Change field UI so that adding a field is a separate task happens.

The hook added here is actually https://api.drupal.org/api/drupal/core!includes!entity.api.php/function/..., and it's already tested by https://api.drupal.org/api/drupal/core!modules!system!tests!modules!enti...

Gábor Hojtsy’s picture

Issue tags: +blocker

Tag as blocker for config translation as per above.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for answering my questions @amateescu

Committed faaaaae and pushed to 8.x. Thanks!

yched’s picture

Nice git hash.

Gábor Hojtsy’s picture

Issue tags: -sprint

Remove sprint tag.

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