Remove unnecessary query parameter

[geichelberger]

The withmetadata=true query parameter is a heavy operation and is pretty slow on receiving multiple events. The result is not used, so we can safely remove that parameter.

We use that patch currently in production, and we found no problems during testing. However, we are only using a subset of the features.

[lukasnoebauer]

We are also using this patch in production, which reduced course load times significantly!

[NinaHerrmann]

@lukasnoebauer Thanks for your contribution could you state which plugins you are using? I just want to make sure metadata is used really nowhere!

[lukasnoebauer]

@lukasnoebauer Thanks for your contribution could you state which plugins you are using? I just want to make sure metadata is used really nowhere!

We are using:

• block_opencast
• filter_opencast
• local_chunkupload
• repository_opencast
• tool_opencast in "Release_v4.1" Version with Moodle 4.1.5

[NinaHerrmann]

Thank you for your fast reply! I think local_och5p/local_ochhvp might be the only plugin that still uses metadata, so maybe @ferishili might have time to make a final comment on it(?). :)

[ferishili]

Hi Nina, thanks for your review. I will check ASAP and inform here.

[aliyevTU]

@ferishili @geichelberger Hello, any update on this? Do we know when it will be merged and new version will be released? Its also affecting our prod.

[ferishili]

Hi @aliyevTU I will discuss this with @MatthiasKollenbroichWWU, and we will sort it out. The release date is not set yet, but you will hear it from us soon.

Thanks BR

[berthob98]

Whats the state of this PR? We are also using this in production for quite some time

[ferishili]

It is currently under development, and will be provided/available in the next release, the upcoming changes will contain a more efficient way of handling this situation as suggested:

instead of omitting withmetadata parameter, I would suggest to define it as false. In future versions of OC, devs can arbitrarily assign another default value to omitted parameters causing again the same problem.

So, basically this PR will be closed and replaced by the related commits soon.

[geichelberger]

That the default behavior is an expensive API request does not make sense.

get_block_videos($courseid, $withmetada = true) https://github.com/Opencast-Moodle/moodle-block_opencast/blob/e2d6194eff1b88769b27cfb839c5fa1066597c87/classes/local/apibridge.php#L304 get_series_videos($series, $sortcolumns = null, $withmetadata = true) https://github.com/Opencast-Moodle/moodle-block_opencast/blob/e2d6194eff1b88769b27cfb839c5fa1066597c87/classes/local/apibridge.php#L410

[ferishili]

We are talking about reusability of those methods for future, and removing them on the fly without any clue is not an option!

[geichelberger]

I am talking about the recent commits to master. The default behavior should be withmetadata=false; the most essential metadata is already part of the response.

The objections would have been excellent to discuss in the PR instead of ignoring the PR for two months.

[ferishili]

The commits are not final yet, and the adjustments will follow, and by the way, nobody ignored the PR, it just overlapped with holidays and sick-leaves! And the objections / suggested change requests were there for 3 weeks!

[berthob98]

It is currently under development, and will be provided/available in the next release, the upcoming changes will contain a more efficient way of handling this situation as suggested:

instead of omitting withmetadata parameter, I would suggest to define it as false. In future versions of OC, devs can arbitrarily assign another default value to omitted parameters causing again the same problem.

So, basically this PR will be closed and replaced by the related commits soon.

There was a release 6 days ago thats why I asked withmetadata=false instead of omitting makes sense

[aliyevTU]

So whats the status there? Is the last week's release fixed this issue or not? Who have any problem with setting withmetadata=false ?

[ferishili]

This is getting really confusing, I think it is time for @MatthiasKollenbroichWWU to explain a bit here!

[ghost]

The release of the last week for Moodle 4.2 does not fix this issue. The fix will be included in the release for Moodle 4.3 (no release date set) and maybe will be cherry picked into the release for Moodle 4.2 afterwards.

Before there are questions again: The release for Moodle 4.2 was planed around 6 weeks earlier, but I had not the time to publish it in combination with vacation and a longer sick leave. The testing etc. for that release was completed already and that's why this PR here, namely its behavior, was not included into that release.

[geichelberger]

@aliyevTU In my opinion, your suggestion was valid, and I would have implemented the proposed change and set the parameter to false instead of removing it.

However, I was waiting for feedback on my PR if I had overlooked something and the metadata is needed somewhere I didn't check. Now, if the metadata is required, we should have verified whether the metadata from withmetadata was necessary, and then I could have implemented a parameterized method.

Outside of block_opencast, I only found the usage of the method get_series_videos in the h5p plugin, and I think it does not use that metadata either, so I conclude that the Moodle plugins do not require this metadata.

Nonetheless, I think the default for the parameterized method should be withmetadata=false as this is a costly operation, and we should avoid it at all costs if it's not called for.

I amended the PR to reflect the new changes from the master branch.

[ghost]

The changes of this PR have been integrated into the master branch finally (see https://github.com/Opencast-Moodle/moodle-block_opencast/commit/d608ec275126874aa471f8dd544d66ea06704425). We've checked and tested the affected locations/features. Therefore, this PR will be closed, now.