Do not allow to alter Locked field via UI

That's weird, that would mean #2150179: Delete confirm form for locked fields is hotlinkable did not fix this.

Oh, right, editable. Sorry, I misread that one completely. However, this behaviour has been there since D7 (maybe CCK as well, but I should check).

Since we support 'not deleting', we could probably support not editing too I guess, but maybe that should be split up then, because the ability to change the cardinality is valuable IMO. OTOH, the permission system could also just hide field ui fields completely for people - even per entity type.

In fact, with the configuration system now in place, the locked property on a field could probably just be removed and then using https://drupal.org/project/config_readonly you can lock your entire site completely of writing, updating or deleting any kind of config entity.

Your code sample creates a configurable field, which by design can be reconfigured by the user. You can add non-configurable fields, which cannot be edited or deleted by the user, to any entity with hook_entity_base_field_info()/_alter() or to a specific bundle with hook_entity_bundle_field_info()/_alter().

"locked" is used only to prevent changes on field via Field UI.
But seems this broken and changing cardinality could lead to data loss! e.g. when changing vocabulary.

Steps to reproduce:
1) standard profile Article node type has Tags
2) change field config drush @d8 cedit field.storage.node.field_tags to locked: true
3) at admin/structure/types/manage/article/fields click "Term Reference" link

4) You can change cardinality and vocabulary

added to summary

A quick fix - disable a whole form so settings could be viewed but no way to change them.

Patch in #9 works but I feel that disabling the form is not as good as they did in "FieldEditForm" Class. So I made a patch in the same way as there. What I don't appreciate is that is copy & paste and by this a code stench. But everything else would explode the scope.

Adding an interdiff

Tagged

Fixed access for edit, still needs tests

@andypost I am trying to reproduce this bug but I am confused in step #2 of reproduce. Can you please elaborate? Thanks

@Shuvam this is drush way to change config directly (set property to locked)

@ Andypost This patch is still not working. I can still access delete from UI which should be banned once patch is applied.

Yes, but we need to mark this fields locked anyway.
Also we need to hide ability to delete locked fields with field ui and cover with tests

It worked : Tested patch #14 and followed
Steps to reproduce:
1) standard profile Article node type has Tags
2) change field config drush cedit field.storage.node.field_tags to locked: true
3) at admin/structure/types/manage/article/fields click "Term Reference" link

Attaching the screenshot for reference.
I think it's solves our purpose of hiding the ability to delete locked fields with field ui.
Working on writing the test for same.

@andy I have restructured the locked field condition.
working on writing test for same!

The test is already written Drupal\field_ui\Tests\ManageFieldsTest for

function testLockedField()
$edit_link = $this->xpath('//tr[@id=:field_name]/td[4]', array(':field_name' => $field_name));
 $this->assertFalse(in_array('edit', $edit_link), 'Edit option for locked field is not present the UI');

This will solve our purpose.

Patch looks great, just needs tests and fix nits

1. +++ b/core/modules/field/src/FieldConfigAccessControlHandler.php
   @@ -23,8 +23,8 @@ class FieldConfigAccessControlHandler extends EntityAccessControlHandler {
   -    if ($operation == 'delete') {
   -      $field_storage_entity = $entity->getFieldStorageDefinition();
   +    $field_storage_entity = $entity->getFieldStorageDefinition();
   +    if (in_array($operation , array('update', 'delete'))) {
   

   getFSD() should stay within if

2. +++ b/core/modules/field/src/FieldConfigAccessControlHandler.php
   @@ -34,5 +34,4 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   -
    }
   

   needs revert

Needs re-roll after #2446869: Convert the "Field storage edit" form to an actual entity form got in.

Patch rerolled and changes from [#26] implemented.

Accidentally added the original of the renamed file. Removed it.

Trying to fix the errors. Hope this works!

@ivanjaros
You know how core development works, we need reviews: https://www.drupal.org/patch/review

Needs tests.

Writing tests for this one

Added tests for this.

Woops, forgot 1 line

Test looks ok to me.

I'd like a sub system maintainer to chime in with a +1 since @swentel has not commented on this issue since #4. It makes sense to me but I don;t have the history with the field system that @yched and @swentel and others do.

Looks good to me, two small nitpicks in current patch.

1. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
   @@ -532,6 +532,11 @@ function testLockedField() {
   +    $this->assertText('The field ' . $field_name . ' is locked and cannot be edited.');
   

   Should use format_string() for replacing $field_name

   </li>
   <li>
   <code>
   +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -137,7 +137,8 @@ class FieldStorageConfig extends ConfigEntityBase implements FieldStorageConfigI
       * - field settings cannot be changed,
   -   * - new fields cannot be created
   +   * - new fields cannot be created,
   +   * - storage settings cannot be changed,
   

   Let's merge this in one line: 'field and storage settings can not be changed'

Let's also add an assert in ManageFieldsTest::testLockedField() to make sure the 'Storage settings' link is not available, should be a simple one line.

Fixed feedback from #49

Fixed feedback from #49

+++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
@@ -530,8 +530,15 @@ function testLockedField() {
+    $this->assertText(format_string('The field !field_name is locked and cannot be edited.', array('!field_name' => $field_name)));

Probably better to use SafeMarkup::format() and it needs to use the placeholder %<code> indicator and not <code>!.

And assertRaw() instead of assertText()

Probably better to use SafeMarkup::format()

Missed this, fixing that now

Alright, looks good now.

Removing SafeMarkup::format() again because #2549805: Remove all usage of FormattableMarkup in tests apart from explicit tests of that API

@geertvd hold fire with that - we're exploring the issue haven't made minds up about the overall direction.

Ok, re-uploaded the patch from #57 and setting back to RTBC

Locked fields have always been a bit blurry :-/

The intent in D7 was to allow modules to define "programmatic fields" for their business logic, and make sure they were not removed by site admins using Field UI, but keeping more mundane aspects (mostly widget and display configuration, but also label...) adjustable.
So you could not *remove* the "field instance", but still freely edit it.

In D8, widget and display configuration are out of the field definition anyway, so that part is not an issue.
I still think it's a pain to forbid changing (or translating, for that matter) the label, since that's really only presentational preferences.
Not sure about the other stuff like field settings, default value, it's probably ok to keep them locked, so that the "semantics" of the field is locked - but then again some field types have settings that can be fairly presentational as well, there's no real way to tell generically

Ideally, we'd move from "locked TRUE / FALSE" to a more granular feature like "this and this are locked, the rest is editable", but we won't go there in D8.

Is there a way we could leave the label editable ?

I can understand the need for #64 but this patch only covers field storage settings, which in my opinion do need to be locked if the field is locked.
Maybe we can create a follow-up for #64?

this patch only covers field storage settings

Well, it does add a check on 'locked' to FieldConfigAccessControlHandler, so if I'm not mistaken this would block access to the "field edit form" for locked fields ?

Oh yeah you are right, nevermind my previous comment then :)

Is there a way we could leave the label editable ?

This is a really good question. It should be editable. it doesn't make sense that you could change the translation but can't change the actual label.

Thanks @yched

Se we want to lock the storage but keep the instance editable

That was the behavior in D7. So this might be the safest at this point ?

As I said in #64, locking all the field (instance) except label might be OK too, but that's a larger departure (and arguably less of a bugfix). No strong opinion, honestly.

Something like this maybe? This leaves the instance editable.

Seems like those link asserts where false positives, fixed that.
I also added an extra check to make sure that the label field is available on the settings form.

bump

looks good to go
@yched any more suggestions?

Hmm, the UI might be confusing to users.

• The Storage tab is visible and linkable, but there's no link on the manage fields page
• When the field is locked, there's still a 'Save field settings' button - which works, but won't do much if there's any other element available there.

Not sure what the best thing is here. Bringing back the 'storage' edit link seems logical, and less hard then trying to figure out whether there's other form elements, which are accessible or not (because they might even be disabled because of stored data) to hide.
Other option might be to add a dedicated access control handler for the storage to just completely hide that tab and not care about any other storage setting.

Not sure, don't want to hold this up.

Woops, didn't want to RTBC it yet after testing.

Yep, if the field storage is locked, the corresponding tab (that is misnamed "field settings" in current HEAD, there is an issue for that) should be hidden as well.

Other option might be to add a dedicated access control handler for the storage to just completely hide that tab and not care about any other storage setting.

I tried this, I added a EntityAccessControlHandler for FieldStorageConfig but field_storage_config is not available in the route path for the storage settings ($path/fields/{field_config}/storage) so this is not working.
Any pointers how to do this?

Let's see how it works with tests (tabs somehow broken in my install)

@yched I think isDeletable() should care about locked status

also discovered that field module uses permissions defined in field_ui module!

Fix access for links and bring a bit sanity to building

@andypost, from what I could tell this is failing because entity_access is expecting the field_storage_config entity to be in the route.

See \Drupal\Core\Entity\EntityAccessCheck

    // Split the entity type and the operation.
    $requirement = $route->getRequirement('_entity_access');
    list($entity_type, $operation) = explode('.', $requirement);
    // If there is valid entity of the given entity type, check its access.
    $parameters = $route_match->getParameters();
    if ($parameters->has($entity_type)) {
      $entity = $parameters->get($entity_type);
      if ($entity instanceof EntityInterface) {
        return $entity->access($operation, $account, TRUE);
      }
    }
    // No opinion, so other access checks should decide if access should be
    // allowed or not.
    return AccessResult::neutral();

Filed child issue #2562855: Fields cannot be administered without Field UI module

@geertvd entity is there, otherwise entityform will not work

+++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
@@ -79,7 +79,7 @@ protected function alterRoutes(RouteCollection $collection) {
           array('_entity_form' => 'field_storage_config.edit') + $defaults,
-          array('_permission' => 'administer ' . $entity_type_id . ' fields'),
+          array('_entity_access' => 'field_storage_config.update'),

The entity is here exists, see \Drupal\field_ui\Routing\FieldUiRouteEnhancer

I think the field_config entity is there, the field_storage_config entity is not.

Entityform is working because of this

 /**
   * {@inheritdoc}
   */
  public function getEntityFromRouteMatch(RouteMatchInterface $route_match, $entity_type_id) {
    // The URL of this entity form contains only the ID of the field_config
    // but we are actually editing a field_storage_config entity.
    $field_config = FieldConfig::load($route_match->getRawParameter('field_config'));

    return $field_config->getFieldStorageDefinition();
  }

1. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
   @@ -525,16 +525,32 @@ function testLockedField() {
   +    // Check that the links for delete and storage settings are not present.
   +    $delete_link_path = $edit_link_path . '/delete';
   +    $delete_link = $this->xpath('//ul[contains(@class, dropbutton)]/li/a[contains(@href, :path)]', array(':path' => $delete_link_path));
   +    $this->assertEqual(count($delete_link), 0, 'Delete option for locked field is not present in the UI');
   +    $storage_link_path = $edit_link_path . '/storage';
   +    $storage_link = $this->xpath('//ul[contains(@class, dropbutton)]/li/a[contains(@href, :path)]', array(':path' => $storage_link_path));
   +    $this->assertEqual(count($storage_link), 0, 'Storage option for locked field is not present in the UI');
   +
   +    $this->drupalGet($edit_link_path);
   +    $this->assertResponse(200);
   +    // Make sure the label field is available even when the field is locked.
   +    $this->assertField('label', 'The label field is available for the locked field.');
   +    $this->drupalGet($delete_link_path);
        $this->assertResponse(403);
   +    $this->drupalGet($storage_link_path);
   +    $this->assertResponse(200);
   +    // Make sure the cardinality field is not available once the field is
   +    // locked.
   +    $this->assertNoField('cardinality', 'The cardinality field is not available for the locked field.');
   +    $this->assertRaw(SafeMarkup::format('The field %field_name is locked and cannot be edited.', array('%field_name' => $field_name)));
   

   The line spacing seems a bit random, can we keep some visual structure here ?

   Also, the comments should make it clearer when we're testing the "field settings" form or the "storage settings form"

   Also, the test that checks the "storage settings" form on a locked field expects a 200 ? I would have thought a 403 ?

2. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
   @@ -168,6 +172,7 @@ public function getDefaultOperations(EntityInterface $entity) {
   +      $operations['edit']['attributes']['title'] = $this->t('Edit field settings.');
   
   @@ -175,16 +180,17 @@ public function getDefaultOperations(EntityInterface $entity) {
   +      $operations['delete']['attributes']['title'] = $this->t('Delete field.');
   ...
   +        'title' => $this->t('Storage settings'),
   

   Labels should be consistent (verb + complement ?)

3. +++ b/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
   @@ -62,6 +62,17 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Forbid access to the form if the field is locked.
   +    $field_storage = $form_state->get('field_config')->getFieldStorageDefinition();
   +    if ($field_storage->isLocked()) {
   +      $form['locked'] = array(
   +        '#markup' => $this->t('The field %field is locked and cannot be edited.', array('%field' => $field_label)),
   +      );
   +
   +      return $form;
   +    }
   

   Do we still need this now that the route is not accessible ?

Fixed #90.2 and #90.3, tests not touched
Cleaned-up implementation, and fixed bug in list builder

New question - is it ok that \Drupal\field\FieldConfigAccessControlHandler uses isLocked() method of storage?

The remaining issue that no tabs displayed for field settings forms

@geertvd finally get you point about entity!

1. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
   @@ -79,7 +79,7 @@ protected function alterRoutes(RouteCollection $collection) {
            $route = new Route(
              "$path/fields/{field_config}/storage",
              array('_entity_form' => 'field_storage_config.edit') + $defaults,
   -          array('_permission' => 'administer ' . $entity_type_id . ' fields'),
   +          array('_entity_access' => 'field_storage_config.update'),
   

   really interesting how form is found here, there's only "field_config" entity on route

2. +++ b/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
   @@ -62,6 +62,7 @@ public function form(array $form, FormStateInterface $form_state) {
   +
   

   todo: remove

Perhaps we can add the field_storage_config entity in \Drupal\field_ui\Routing\FieldUiRouteEnhancer?

This solves the field ui issue from #2650898: ListBuilders do not check $entity->access() for operation links. Marking as a contrib blocker, as I've got a module in development that currently ends up with lots of errors and an empty default operation due to this issue.

I couldn't quite tell what was left to be done on this. Very happy to do some work on pushing this forward if someone is able to point me at what's remaining.

This will be very useful for domain access module and domain_entity (access) modules to disallow changes on automatically assigned domains to entities

Discussed this today with @tstoeckler, @amateescu, @plach and @xjm and agreed that this is not a major issue. You need to manually craft a URL to be able to access a page where you can change those settings. And even if we lock this down then there are always way around it, like drush cedit or config export/import in the UI.

What we need to figure out and define is what Locked exactly means, as we weren't sure what exactly should be allowed and what not.

There was also the idea to have a less strict version that only prevents from deleting a field. But that can also be done using entity access.

Another related problem that I recently noticed is that a locked field can't be re-used. I think there isn't really a reason why that shouldn't work, we can either fix that here or create a separate issue.

You don't need to manually craft a URL to be able to access a page where you can change field storage settings for locked fields. An example:

1. create an entityreference field as locked (you'll need to provide some more info like 'settings' => [ 'target_type' => 'node'], since an entityreference field MUST know what type of entities it can reference, and it's impossible to create an entityreference field manually without such information);
2. make sure field_ui.module is enabled (but hey, it's impossible to create fields manually without this anyway);
3. visit the "Manage fields" page of the entity you've added this entityreference field to (eg. http://example.com/admin/structure/types/manage/page/fields);
4. the third column (titled "Field type") contains an "Entity reference" link for this field (regardless the fact that the fourth column titled "Operations" says "Locked" for this field).

Clicking on that link allows changing the entity type this locked entityreference field can reference (at least without the patch).

Not sure if this warrants the major priority, tho.

I think the biggest question would be what part of the field should be locked? The field storage settings? The field instance settings? Both? Even something more like deletion of the field as well? Even something less like only disallowing deletion of the field? Any combination of these?

Updating title as per proposed solution/problem statement as it's causing confusion in #2831936: Add "File" MediaSource plugin

Hmm, that sounds like a flexible solution, but would result in a HUGE permission maze pretty quickly.

what Locked exactly means,

The related issue

IMO this property should be renamed to field_ui_locked

Re-roll patch created for 9.1.x, Please review.

Fixed tests, Please review.

Closed https://www.drupal.org/node/806102 as a duplicate of this. Please move over credit for
RoSk0
mdm
chr.fritsch

@smustgrave, thank you. I have reviewed the now closed duplicate and moved credit.

So patch #123 didn't work for me

I locked a field but when viewing I could still edit the widget and based on the issue summary that should not be the case. If that's no longer the proposed solution then the issue summary could use an upate.

Also added a small test assertion.

@smustgrave, thank you for your patch in #138! I just tested it and it works as expected! :) RTBC+1

BUT:
I disagree it's the final solution and don't think #806102: Locked fields can change the widget but not settings should have been closed as duplicate. This issue is only about disallowing to edit the field storage related configuration like cardinality. That's totally fine and a clear bug that should be fixed!

But on the other hand, and that was also an aspect of the other issue, the locked: true on field storage configuration currently also locks editing field instance related configuration, like field instance

• label
• description
• default values

which is also an important issue. Especially not being able to edit those on reused fields with a locked field storage is a problem we run into again and again and end up editing the field instance configuration directly. That shouldn't be the case.

So I decided to create a separate issue for that: #3345006: Locking field.storage should not lock field instance settings (field.field) instead of reopening #806102: Locked fields can change the widget but not settings. I hope that's more specific.
Both should be solved.

Updating credits since I re-opened #806102: Locked fields can change the widget but not settings.

I agree that the current logic is kind of wrong as argued by #113. However, the config entities being separate for field storage and field instance tends to be more of an implementation detail. I can't think of a use case where users would actually want to customize permissions for these two forms separately.

I think it might make sense to hold off making this change because we are looking into combining these two forms in #3347291: Combine field storage and field instance forms. Keeping the access controls tied together like it is currently, would be simpler from that perspective.

I'd recommend that we keep the keep the access control to two of these config entities connected, and we add additional access control to \Drupal\field\FieldConfigAccessControlHandler::checkAccess to prevent editing locked field configs.

@lauriii:

I can't think of a use case where users would actually want to customize permissions for these two forms separately.

That might be true. The point is, that if it's separated in the data model, it's at least possible and I'm not sure we should assume that. On the other side it is additional work. Wondering who we could ask.

One typical example that happens very often for us is, that the default value should not be locked, in contrast to all other field stuff. The default value is often very site-specific, while the fields / schema is general.
As written above, it's currently locked by the form, as it's in the wrong place. Locked by the the field storage lock in the UI, but belonging to the field instance, that should not be locked. See #140.

I think it might make sense to hold off making this change because we are looking into combining these two forms in #3347291: [PP-1] Combine field storage and field instance forms.

Agree, would be nice, if the points from about could be taken into account.