Hi,

We just upgraded a site from D7 to D8, great work getting H5P up and running so easily! However, we're hitting a configuration management issue in that the last update/last check times in the configuration gets updated daily. Because we check our sites for configuration changes every night, this leads to undue attention spent on sites with h5p.

But more importantly, these values are not really configuration -- they are state.

-h5p_content_type_cache_updated_at: 1515472685
+h5p_content_type_cache_updated_at: 1515566770
h5p_site_type: internet
-h5p_fetched_library_metadata_on: 1515472685
+h5p_fetched_library_metadata_on: 1515566770
h5p_first_runnable_saved: 1

... I suggest removing these items from the configuration, and instead storing in the State API. This will make configuration management far easier on sites that have multiple copies -- otherwise you get merge conflicts, and all sorts of noise!

I'll create a patch in the next few days if nobody beats me to it...

CommentFileSizeAuthor
#22 h5p-settings-updated-too-often-2936079-22.patch484 bytesabdhomsi
#16 h5p-settings-updated-too-often-2936079-16.patch3.31 KBfreelock
#15 h5p-settings-updated-too-often-2936079-15.patch3.38 KBfreelock
#6 h5p-settings-updated-too-often-2936079-6.patch4.63 KBfreelock
#3 h5p-settings-updated-too-often-2936079-3.patch4.45 KBfreelock

Comments

freelock created an issue. See original summary.

thomasmars’s picture

thomasmars commented

Hi,
This sounds very reasonable, we won't be able to fix this ourselves currently, but we'll happily accept a patch.

freelock’s picture

freelock
Location Seattle
commented
Status: Active » Needs review
StatusFileSize
new h5p-settings-updated-too-often-2936079-3.patch4.45 KB

Here you go!

Thanks,
John

thomasmars’s picture

thomasmars commented

Thank you for the patch.
Could we transfer/keep the old config, and move it to state in the upgrade script ? Correct me if I'm wrong, but the way it is implemented now, you will lose your old data for when you last updated content type cache and fetched libraries metadata.

freelock’s picture

freelock
Location Seattle
commented

Hi,

Sure we can move the config values into the state... It just didn't seem important, as without it it just re-fetches the library and updates the content type cache (and sets the time in the state api).

I'll go ahead and update the patch.

Cheers,
John

freelock’s picture

freelock
Location Seattle
commented
StatusFileSize
new h5p-settings-updated-too-often-2936079-6.patch4.63 KB

Updated patch for #4.

thomasmars’s picture

thomasmars commented

Thanks for the update, looks great. I have created an issue for reviewing and merging this in at https://h5ptechnology.atlassian.net/browse/HFP-1818, hopefully we can get this merged in quite quickly.

paalj’s picture

paalj commented

Hi freelock,

I have been looking into this patch, and I see you have done changes in vendor/h5p/h5p-core/h5p.classes.php and vendor/h5p/h5p-editor/h5peditor-ajax.class.php. Unfortunately, you can't add any Drupal dependencies to the these libraries (h5p-core & h5p-editor), since they are in use in several PHP implementations (eg WordPress & Moodle)

Each platform implements the set/getConfig, and there is currently no set/getState. What you could do, is to change the Drupal implementation of set/getConfig to use the state instead of the config for these two fields only. Then there would not be any needs for the changes you have done in src/H5PDrupal/H5PDrupal.php.

paalj’s picture

paalj commented
Assigned: Unassigned » paalj
freelock’s picture

freelock
Location Seattle
commented

@paalj ok, I can make these changes in a few days if you haven't gotten to it... but I'm assuming you're making these changes if it's assigned to you?

freelock’s picture

freelock
Location Seattle
commented

P.S. any thought to moving the vendor library out of the module, and managed as a library instead?

falcon’s picture

falcon commented
Assigned: paalj » Unassigned
falcon’s picture

falcon commented

@freelock we have so far prioritized making it as easy as possible to install H5P instead of going the library route, also since it is very unlikely that other modules will use the libraries we're using without using the H5P modules. We're also creating and maintaining those libraries ourselves.

paalj’s picture

paalj commented

@freelock: Sorry, I am not sure why I assigned it to me :(

If you update the patch, I will review and test it!

freelock’s picture

freelock
Location Seattle
commented
StatusFileSize
new h5p-settings-updated-too-often-2936079-15.patch3.38 KB

Updated patch to confine the changes to the Drupal h5p class, and not change the vendor libs. Now getOption and setOption check a list of options to store in the State API instead of the config API, and this list contains the two timestamp options...

Cheers,
John

freelock’s picture

freelock
Location Seattle
commented
StatusFileSize
new h5p-settings-updated-too-often-2936079-16.patch3.31 KB

Whoops, missed the change to the constant names in the update function...

  • paalj committed 6eb82c7 on 8.x-1.x authored by freelock 
    Issue #2936079 by freelock: h5p_settings gets updated too often
paalj’s picture

paalj commented
Status: Needs review » Fixed
paalj’s picture

paalj commented

@freelock: Thank you for your patch. I was not able to find any problems with it, so it will be part of the next H5P D8 release.

freelock’s picture

freelock
Location Seattle
commented

Thanks!

Status: Fixed » Closed (fixed)

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

abdhomsi’s picture

abdhomsi commented
StatusFileSize
new h5p-settings-updated-too-often-2936079-22.patch484 bytes

You will get a PHP error If you enabled the config_readonly module along with H5P, because H5P module try to update own configuration.
Is there any chance to store the following using State API too?
- h5p_site_uuid
- h5p_first_runnable_saved
- site_type

freelock’s picture

freelock
Location Seattle
commented

@abdhomsi I'm not a maintainer here, but I would highly recommend opening a new issue for this, and add your patch there, rather than adding to a closed issue which might not get noticed...