Problem/Motivation
The config entity type config_prefix
property may form a part of a config entity's file name -- the yml
file that represents an instance of the entity. The file name length is limited to 255 characters by most operating systems. To be safe, we limit the name to 250 characters and leave 5 for a file extension. The file name is potentially composed of 5 pieces. Each part may therefore only consume 20% of the available characters, so the config entity type config_prefix
can therefore only be a maximum of 50 characters long.
Here is an example of an config entity type config_prefix
in Core using the EntityFormDisplay
config entity.
/**
* @ConfigEntityType(
* id = "entity_form_display",
* label = @Translation("Entity form display"),
* config_prefix = "form_display",
* entity_keys = {
* "id" = "id",
* "status" = "status"
* }
* )
*/
Proposed resolution
Config entity types with a config_prefix
greater than 50 characters must:
- Fail installation
- Prevent the owning module from installing as well.
This will keep contrib authors from creating config entity types with config_prefix
greater than 50 characters and discourage adding them to D.O.
User interface changes
None.
API changes
Perhaps new methods.
Original report by @jessebeach
Comment | File | Size | Author |
---|---|---|---|
#37 | limit-config-prefix-length-2220749-37.patch | 7.82 KB | jessebeach |
#37 | interdiff.txt | 3.59 KB | jessebeach |
#34 | 2220749-34-limit-config-prefix-lenth.patch | 7.1 KB | gremy |
#23 | 2220749-23-limit-config-prefix-length.patch | 6.2 KB | gremy |
Comments
Comment #1
xjmI think our goal is ultimately to get rid of config_prefix as a separate concept entirely through #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term), so pursuing that goal foremost might be a better way to tackle this.
Comment #2
jessebeach CreditAttribution: jessebeach commentedClosing as a duplicate of #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term).
Comment #3
jessebeach CreditAttribution: jessebeach commentedReopening this as a replacement for #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term), which is now postponed to 9.x.
By limiting the combo of extension name and config_prefix/entity_id to 83 characters, we leave 166 characters (combining to a maximum of 250 characters) for a config entity file name.
The 83 character limit is imposed as a combination in order to allow for short module names and long config_prefixes/entity_ids or the reverse.
Comment #4
gremy CreditAttribution: gremy commentedComment #5
gremy CreditAttribution: gremy commentedAdded a new exception, ConfigPrefixLengthException, that is thrown in getConfigPrefix() if the output length is more than 83 characters.
Comment #6
gremy CreditAttribution: gremy commentedRerolled patch: changed strlen to drupal_strlen
Comment #7
dawehnerI think you can just use $entity_type = new ConfigEntityType ... in the test functions and get rid of this
Note: There is also @expectedExceptionMessage to check the message. Your message is not absolute trivial, so it would be nice to also test that
Nitpick: here are two extra lines.
Comment #9
gremy CreditAttribution: gremy commentedRerolled patch based on feedback.
Comment #10
BerdirMissing . at the end I think?
The comment here is now misplaced, as the action that it describes has been moved up. The comment should probably move to the beginning of the method.
There's one additional combination of what that function does, and that's specifying a provider + id without config_prefix. Should we add a test for that as well while we're at it?
Comment #11
gremy CreditAttribution: gremy commented1. No missing . at the end. That should be in the comment and it is there.
2. Moved comment
3. Not the scope of this issue. The tests in this issue check if based on the length of the config prefix we the exception is thrown.
Comment #12
gremy CreditAttribution: gremy commentedComment #14
BerdirWell, if it throws (not returns) an exception, then obviously we get one. This should say something like "Tests that we get an exception when the config prefix is too long", and the one below "... when the config prefix is not too long". Explain the why.
Also, both *are* missing a . at the end of the line. As is the exception description.
One of those descriptions is lying, 58 / 60 but 84/83 ;)
Also, those comments need to be formatted according to our coding standards, which means on their own line, start with a upper case character should be a full sentence and ending with a ".".
I would write this as a single sentence at the top of the method: "// A config entity with a provider length of N and config_prefix length of N (+1 for the .) results in a config length of N, which is too long."
Make sure to wrap it 80 characters.
Comment #15
BerdirNote that the test fails were a bot problem, you can ignore them.
Also, I would still argue that testing it with id makes sense because that could be too long too. While that does not matter for the current implementation, it could also be written like differently and we should be making sure every possible combination is verified.
Comment #16
gremy CreditAttribution: gremy commented12: 2220749-10-limit-config-prefix-lenth.patch queued for re-testing.
Comment #18
gremy CreditAttribution: gremy commentedRe-rolled patch based on feedback from Berdir
Comment #19
gremy CreditAttribution: gremy commentedComment #20
gremy CreditAttribution: gremy commentedComment #21
BerdirThanks, almost there :)
Wondering if we want to use $his->randomName(24) and so on for those strings, would in this case be more self-explaining than coming up with strings of that length ourself?
Maybe "Valid" instead of "Normal"?
Comments look fine now, only problem is that some of them are over 80 characters wide, which they shouldn't.
Not sure what to about the method description, maybe rewrite it a bit to something like "Tests that a valid config prefix does not throw an exception"?
Also, the method descriptions for the ID methods still mention "config prefix".
Comment #22
gremy CreditAttribution: gremy commented1. At first I used $this->randomName(), but in order to be able to use @expectedExceptionMessage I switched to using made-up strings. I could however use it for 2 out of the 4 tests.
2. Sounds better with Valid ... I'll do that
3. I'll see what I can do
Comment #23
gremy CreditAttribution: gremy commentedComment #24
BerdirThanks, made some small changes to the comments and switched to 59 instead of 60 so that we actually test the exact number we're checking.
This looks good to me, just needs an RTBC I think :)
Comment #25
jibranThank you tests look fine code change make scene RTBC.
Comment #26
jessebeach CreditAttribution: jessebeach commentedWe should declare a constant for this limit rather than hard-coding 83.
The error message is misleading. It's not that
config_prefix
is too long at more than 83 characters; the 'configuration file name prefix' is too long. We should express the difference of provider name length and the config_prefix/id name length as the limit. Maybe something like:'The configuration file name prefix @config_prefix exceeds the maximum character limit of @max_char'
Comment #27
BerdirOk, improved the exception message and added the constant.
Comment #28
jessebeach CreditAttribution: jessebeach commentedI reviewed with Berdir in person. We're good to re-RTBC when it comes back green.
This issue is enough of an 8.x-8.x change that we need a change notice. I've started that here: https://drupal.org/node/2227275
Comment #29
jessebeach CreditAttribution: jessebeach commentedChanges made. Tests passed. Change record is ready. Back to RTBC.
Comment #30
xjmLet's have @alexpott take a look at this one.
Comment #31
alexpottIt would nice to document why we've chosen 83 here.
Missing full stop.
Comment #32
alexpottThese tests could be combined and use a data provider.
Comment #33
BerdirShort explanation of what we discussed/I suggest:
For #31.1: Just add a sentence that says, limit prefix to 83 characters so that we have 166 left for the configuration entity ID. That's the only reason we have now, as you're free to split it between module and prefix.
#32:We could even make all 4 test methods use a provider by using setExpectedException(). *That* would actually allow us to use randomName() for those tests as well, because then we can dynamically build th exception message. Will avoid those very long strings and be more self-explaining I think. Look at other tests that have @dataProvider.
Comment #34
gremy CreditAttribution: gremy commentedRerolled patch based on feedback.
Comment #35
gremy CreditAttribution: gremy commentedComment #36
jessebeach CreditAttribution: jessebeach commentedComment #37
jessebeach CreditAttribution: jessebeach commentedJust a few comment tweaks. No code changes. I reviewed the changes with Berdir in person and verified the test cases.
alexpott's #32 and Berdir's #33 comments have been addressed.
Back to RTBC when it comes back green.
Comment #38
jessebeach CreditAttribution: jessebeach commentedThis was green before. I just made comment edits. Back to RTBC.
Assigning to alexpott to put this on his radar.
Comment #39
alexpottCommitted 361ddb1 and pushed to 8.x. Thanks!
Comment #41
xjmRetroactively fixing the component.