Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Discuss.
Comment | File | Size | Author |
---|---|---|---|
#100 | date-2003892-100.patch | 95.68 KB | tim.plunkett |
#100 | interdiff.txt | 804 bytes | tim.plunkett |
#97 | interdiff.txt | 4.97 KB | tim.plunkett |
#96 | date-2003892-96.patch | 95.57 KB | tim.plunkett |
#96 | interdiff.txt | 3.14 KB | tim.plunkett |
Comments
Comment #1
dawehnerComment #2
tim.plunkettBlocked on #2003942: Convert system_configure_date_formats_form() to a controller
Comment #3
tim.plunkettThis does not change datetime.module or any tests, so it will fail.
Comment #4
jibranUpdating the title.
Comment #5
jibran:/
Comment #7
tim.plunkettFixed more stuff.
Will have to talk to someone who worked on #1571632: Convert regional settings to configuration system or #1646580: Implement Config Events and Listeners, and storage realms for localized configuration to find out how to reconcile the locale.config stuff.
Comment #8
dawehnerWe should certainly do some performance testing as loading an entity vs. just loading a config seems to be sort of more expensive.
Comment #9
pounardThinking as an integrator who want to edit his formats directly in the YAML files, in term of usability, date formats as they were originally (in one single YAML file) sounds a lot more nicer to edit than N YAML files where you definitely can't have a global overview of all your formats in one eyeshot.
Does it really worth to replace date formats config file by another config entity? We are generating tons of smaller files VS one single (still small) file. But in the end, we are wasting disk space and polluting config, which then contains more metadata for the entity system than usefull data for the frontend building.
Plus in term of performances, it will probably really hurt in the end: the
format_date()
function will load a different entity for each call (hence one SQL query or one remote cache backend query depending on configuration) per date displayed in current state, and one per different date format being used later once entity static caching will land). This means that we are multiplying the number of remote storage backend for dates from 1 to N where N is number of formats being used on the same page.It does not sound like an improvement at all to me. Is there any use case where having entities for date format would be useful?
Comment #10
msonnabaum CreditAttribution: msonnabaum commentedThis is more about consistency. Most everything else that is a configuration created by a user is a config entity.
I'd prefer to not have performance speculation enter into it. If there's an issue, it can be measured.
Comment #11
pounard1 query for laoding all formats at once VS 1 query per for each format displayed is a simple deduction reading the above patch.
Measures won't show any significant different if you add 2 or 3 queries: there are already almost 200 lying around. Nevertheless if everything is converted the same way in the whole system for the sake of consistency, the number of queries could explode significantly.
Drupal 8 has already 50 @EntityType references I could grep in code, found at least 10 being config entities.. Hopefully some are used only via some procedural accessors loading them all and statically caching them, but not all of them.
So, I installed a fresh D8 (justed pull from git) and added two nodes with different types and one comment on the home page, did an anonymous hit with warmed up caches, and used a *cachegrind to look up at the profiling output: 132 queries show up, ~90 are cache, among them ~40 are cached config read operations, ~10 are config entity loads, and we're about to add some others. I didn't added any other fancy stuff, the home page is pretty empty there. I didn't added any other language than english or blocks, this would make the number of queries up to 200 (approximative numbers from my previous week's tests).
EDIT: Anyway this was just some thoughts about config entities, I'm not against consistency either so let's see what happens in the future for performances.
Comment #12
tim.plunkettMore work on the upgrade path. Still need to fix locale stuff.
Comment #15
tim.plunkettSorry, here's the interdiff.
Comment #17
aspilicious CreditAttribution: aspilicious commented@pounard
With the current approach you can't add any new default formats in yml. You need to add some code in module_install to change the default date config.
ux--
So triple +1 for this patch.
Comment #18
tim.plunkettMore upgrade path test fixes.
Comment #20
tim.plunkettMore fixes.
Comment #24
tim.plunkettOkay!
Comment #26
tim.plunkettWhoops, last minute refactoring gone wrong. This should finally pass.
Comment #27
Gábor HojtsyTim asked me to look at this issue. I must admit I have not looked how language vs. format relations are stored. It looks odd that we'd use the config override system with files of equal content (if I understand right) to the original file, just so we have a list of languages the format applies to?
Comment #28
tim.plunkettCore essentially provides a full config translation interface for date formats.
I honestly don't know how to replicate that for config entities, it would be the first instance.
I think this is actually not as terrible as I thought, and can be revisited once/if #1952394: Add configuration translation user interface module in core goes in.
Comment #29
Gábor Hojtsy@tim.plunkett: yeah my understanding on what core does is it lets you assign the same date format string to multiple languages. It is not really a translation feature. You cannot translate the date format to other languages. A date format either applies to a language or not. So what the code does is it saves the *same* data to multiple files and then uses the filenames as keys to figure out which languages does the date format applies to. This is no doubt creative use of the config translation system.
Anyway, I see this is somewhat of a prior behaviour, although largely adapted here to work in a custom way on top of config entities.
Comment #30
tim.plunkettWhile we figure out how to handle this, rerolling and ensuring the locked functionality stays in.
Comment #31
aspilicious CreditAttribution: aspilicious commentedAren't we missing a test for the locked functioanlity? If it came back green before.
Comment #32
tim.plunkettAdded tests, and moved the locked stuff into an access controller.
Comment #34
tim.plunkettThe EntityManager hunk was also included in the actions patch.
Comment #36
tim.plunkettEntityAccessCheck doesn't work with 'create'.
Also, needed to follow the /manage/ path pattern of other entities.
Finally, used update_7_to_8_install_default_config() instead of config_install_default_config().
Comment #37
vijaycs85Great work @tim.plunkett. I just manually tested by:
1. Applying the patch in #36 in a fresh D8 code base
2. Build site
3. Add a new format
4. Edit default one
Everything works fine exception the localization issue below.
Issues:
1. Selected "Select localizations" is not getting selected/updated properly.
Suggestions:
how bad idea is it to introduce a helper for this... something like below:
(can be part of interface, if it makes sense)
So that we can replace the code with
DateFormat::getFormat($type, $pattern);
in all the places?Comment #38
tim.plunkettFixed the locale form bit, I just left off the default value by mistake.
I'm not sure about adding a static method like that, we don't do that in any other entity types. It would also mean hardcoding Drupal\system\Plugin\Core\Entity\DateFormat::getFormat(), which is not good since entity types are theoretically swappable.
Comment #39
vijaycs85thanks for the update. Tried the same steps in #37 and all working good.
Here is few minor code reviews. Surely not show stoppers :)
Minor:
more than 80 charsEDIT: This is wrong. it is with in 80 chars
Minor: All schema follows label as Ucfirst until there is no UI which has different format. I can see that old schema has word cap, but I don't see this labels in D7. So guess this is introduced in D8. If so can we fix this label?
Minor: Class docblock missing?
Comment #40
vijaycs85Fixed both label and comment issue in #39. Exclude me in commit message, just committing this patch as I've reviewed yesterday and didn't mention #39.
Good to RTBC from my side.
Comment #41
tim.plunkettThat was actually a bad copy/paste job on my part.
Comment #42
vijaycs85As discussed with Tim on IRC, we may need to create a follow up to fix the name (e.g HTML Datetime => Datetime (HTML), Default Short Date => Short date (Default), etc...). So it's RTBC, if test is green.
Comment #44
tim.plunkettKeepin' up with the drop.
Comment #45
larowlanProfiling results attached.
Summary of /user/1
Before is on left
Summary of /node with 50 nodes
Comment #47
tim.plunkett#44: date-format-2003892-44.patch queued for re-testing.
Comment #48
larowlanNote that my profiling was done with xdebug on, which I've been told is wrong, will look at it again tomorrow.
Comment #50
vijaycs85formatted version of #45:
Summary of /user/1
Summary of /node with 50 nodes
Comment #51
tim.plunkettformat_date() is now \Drupal\Core\Datetime\Date::format()
Comment #53
tim.plunkett#51: date-format-2003892-51.patch queued for re-testing.
Comment #54
andypostPerformance looks really bad, not sure that caching should help here.
Otherwise great patch
Comment #55
dawehnerSo at least these two entity_load methods will be loaded all the time. What about moving this to the process function?
Comment #56
tim.plunkettLet's see if this works.
Comment #58
tim.plunkett#value_callback is called before #process. So that's a no go for now.
Rerolling without that.
Comment #60
tim.plunkettThis needs profiling still, as the previous numbers were run with xdebug enabled.
Comment #62
tim.plunkett#60: date-2003892-60.patch queued for re-testing.
Comment #63
tim.plunkettRerolled, it turns out this contained #1987822: Convert system_date_time_lookup() to a new style controller
Comment #64
tim.plunkettThis blocks removal of MENU_LOCAL_ACTION.
Comment #65
msonnabaum CreditAttribution: msonnabaum commentedLooking at a node page with 12 comments, I'm seeing practically no difference in performance.
Attached the two runs I compared if anyone wants to take a closer look.
Comment #66
msonnabaum CreditAttribution: msonnabaum commentedAnd since the previous one was /node with 50 nodes, I just looked at that, and it's also nearly identical.
Comment #67
tim.plunkettStill needs docs work on my part, just rerolling.
Comment #68
tim.plunkettAttempted reroll after language-as-config.
Comment #69
ParisLiakos CreditAttribution: ParisLiakos commentedlooks like a very nice cleanup!
Well, @params and @returns are there but the typical boilerplate docs are missing.
not sure why this is needed? if $locale is string, what about using it as a key? would make the array unique check unneeded
Comment #70
tim.plunkettThe pre-existing exports were also numerically indexed, not associatively, so I'm using that to maintain the previous behavior.
Added the docs.
Comment #71
ParisLiakos CreditAttribution: ParisLiakos commentedwell, manually tested, behavior exactly the same, profiling done, cant find anything to complain about, looks ready to me!
Comment #72
tim.plunkettRerolled around #2024833: Adopt load() and loadMultiple() on entity storage controllers
Comment #74
tim.plunkett#72: date-2003892-72.patch queued for re-testing.
Comment #75
tim.plunkettHEAD was broken #2024833: Adopt load() and loadMultiple() on entity storage controllers
Comment #76
alexpottChanging priority as this is part of #1802750: [Meta] Convert configurable data to ConfigEntity system
Comment #77
vijaycs85Replacing
format_date()
with\Drupal::service('date')->format()
Comment #78
dawehnerIn a perfect world this service would have the entity storage controller injected.
format_date can be replaced here as well.
Comment #79
tim.plunkettThis was RTBC and green, format_date() nitpicks aside. Will reroll tonight.
Comment #80
tim.plunkettThat was a pretty invasive nitpick, let's leave others for follow-ups.
Comment #81
dawehnerNice!
Comment #82
alexpottI'd be tempted to rename this dateFormatStorage as it does not store dates...
Missing uuids from all the default config files.
Missing a dot after system.date_format ... just in case we do something mad like create system.date_formatter.settings.yml
Lets add a dot to $config_prefix for the same reason.
I think these might cause confusion we are in a Field plugin here... the $dateStorage kind of implies that this might store the field... perhaps DateFormatStorage? And I would call $date $dateService just to be clear too.
Comment #83
tim.plunkettGood points all around.
While re-exporting the CMI files, it rearranged the keys a bit, nothing to worry about as I hand wrote them before.
Also found this (unrelated) bug #2034113: System's entity info is not available during drupal_install_system() while trying to give the date formats UUIDs.
Comment #84
dawehnerThe interdiff itself looks great.
Comment #85
larowlannitpick:Is this still valid?
Can't this be injected now? See https://drupal.org/node/2012118
Shouldn't this inject the date format storage controller and load from that without the call to entity_load?
Should this be?
<ignore>damn you user_access! still a blight on our OO code :)</ignore>
should this use $dateService too (see earlier review)
love seeing this!
We can't inject this yet can we?
yeah this confirms my comment above about using entity_load
$this->container->get('plugin.manager.entity')->getStorageController('date_format')->create() instead?
$this->container->get('plugin.manager.entity')->getStorageController('date_format')->loadMultiple instead?
Same again
Comment #86
tim.plunkettNeither widgets nor formatters are using ContainerFactory, so I can't inject anything.
I'm not switching any entity_load()s in tests in a 4xRTBC 90K patch. Someone else can if they want. They're tests.
Comment #87
larowlanSo given that #2034563: Switch field plugin managers to DefaultPluginManager handles injection for field plugins, this is back to rtbc
Comment #88
alexpottWas going to commit but testing and reviewing revealed a couple of things...
the date service is being injected this should be removed...
It is possible to delete all the data formats including medium... if you do this and the go and create an article you will see the following error
Fatal error: Call to a member function getPattern() on a non-object in ./core/lib/Drupal/Core/Datetime/Date.php on line 112
. This is broken in HEAD to but it just does not fail as bad.This is why this happens... I think we need to fix this here because before it broke silently and now it's in your face. Afaics we have two options: (a) default to a locked format such as html_datetime or (b) create an new locked format called fallback. I favour the latter as this means distributions can alter this.
Comment #89
alexpottAnd now #2034563: Switch field plugin managers to DefaultPluginManager is done so some of @larowland's review in #85 is relevant
Comment #90
tim.plunkett#2035317: DateTimeDefaultWidget should implement ContainerFactoryPluginInterface still blocks some of @larowlan's proposed changes.
Wow, FormatterPluginManager::createInstance() makes using ContaineFactoryPluginInterface ugly as sin.
Good call on the medium/fallback bit.
Comment #92
tim.plunkettSigh.
Comment #94
tim.plunkett#92: date-2003892-92.patch queued for re-testing.
Comment #95
alexpottI think we should have a test for the fallback logic
Incorrect usage of camel case in the $dateService arg
Let's call this
'Fallback date format'
to be a bit more explicitComment #96
tim.plunkettThis was the test for the fallback, it was one of the failures in #90, see the interdiff in #92. I have to change the assertion text.
Comment #97
tim.plunkettHere's the full interdiff since the last RTBC.
Comment #98
larowlanLooks good to me
Comment #99
alexpottHmmm... I suspect that the
return;
should bereturn TRUE;
. The interesting thing about this is I suspected that this is denying access at the minute... seeEntityAccessControllerInterface::access
(note this is very different fromAccessCheckInterface::access
method)Comment #100
tim.plunkettOne never "views" a config entity. At least not yet. If entity_reference stops restricting to content entities, then this check will be used.
And yes, I was thinking of AccessCheckInterface :)
#100!
Comment #101
mradcliffeThere is a serious usability issue with the use of #ajax in this form that was introduced in #2003942: Convert system_configure_date_formats_form() to a controller and continued in the AjaxResponse. It is annoying that it does a request for each key stroke (keyup), which disrupts the normal flow of typing.
Can we fix this in this issue as well because it is pretty bad?
Comment #102
tim.plunkettThat behavior is not altered in this patch. Please open a separate bug.
Comment #103
alexpott+1 for getting this in now... anyone willing to re-rtbc? :)
Comment #104
ParisLiakos CreditAttribution: ParisLiakos commentedyeah this ajax thing was there before, i noticed while manual testing
lets get this in
Comment #105
alexpottOpened followup #2038231: Usability issue with the use of #ajax in date format list
Committed 06cb438 and pushed to 8.x. Thanks!
https://drupal.org/node/1876852 needs an update...
Comment #106
Gábor HojtsyThe config schema got pretty outdated with this commit. It should be updated: #2038285: Update configuration schema for date formats as entities.
Comment #107
yched CreditAttribution: yched commentedNot sure what this refers to exactly, but is there an issue opened for this ?
20 days to next Drupal core point release.
Comment #108
yched CreditAttribution: yched commentedunintended status change
Comment #109
tim.plunkettComment #110
catch#2083415: [META] Write up all outstanding change notices before release
Comment #111
andypostIssue to discus a ways to lock (prevent delete) of config entities #2084567: Implement a EntityLockedInterface and service to allow locking entities
Comment #112
Gábor HojtsySee #2127941: Remove two (out of 3) bogus and duplicate date languages UIs for an in-depth rundown of the user interfaces / behaviours and why they are wrong for localization needs (and should be removed in favor of the config translation module).
Comment #113
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
The patch for this issue was committed on July 9, 2013, meaning that the change record updates for this issue have been outstanding for more than six months. Let's get https://drupal.org/node/1876852 updated so that we can close this issue.
Comment #114
Gábor HojtsyUpdated the change notice as requested. https://drupal.org/node/1876852/revisions/view/2503042/6882875