🇵🇱Poland @Graber

Also a bit more.

graber → created an issue.

Thank you, Looks good!

I have one more concern: what if someone deletes a lot of entities - do we still want to log all that? I think we may leave it as is for the time being though and if someone's log will be flooded and they don't like that they can always create a follow-up to make it configurable.

I'll let it lay for a week or so in case someone else had any feedback.

Great but please see my notes on the other MR that also apply.

Ok, I left the review on https://git.drupalcode.org/project/views_bulk_operations/-/merge_request... but it also applies to the latest branch.

Left a review, also please make sure all checks are green.

In general this requires child issues, if we'll start discussing here, we'll have one big mess. Not all apply too as many can be solved with views or other project-specific configuration.

I know this should probably use tokens.. that can be a follow up.

You are able to add a taxonomy tag field to activity types and include it in the activities selection view. Same with lessons, just a bit more tricky as those doesn't have field API exposed anywhere. Activities are also a bit tricky because that field would have to be added to every bundle and there may be a lot of those.. in any case it's something that's very very easy to implement on any project.

Not sure which way should we take here.. one project will require tags, one categories, another some other filtering criteria..

Ahh, yes.

Bumping minor for this so we can remove some old stuff (mainly or only update hooks).

Indeed.
Closing in favor of 📌 Improve module code quality checks Active and going for it.

graber → created an issue.

Ahh, looks much better now, thank you.

Looks good, thank you.

I see this on the other hand. Claro.

graber → created an issue.

graber → created an issue.

graber → created an issue.

As you can see here: https://xapi.com/statements-101/, statements should have the elements that are reported as missing. But I guess there can be different cases so this also needs more invastigation.

We haven't tested with Moodle, the statement data validation happens in Drupal\lrs_xapi\XapiStatementStorage::setStatement() and that's where the errors come from so you need to do some debugging what is is that actually Moodle sends there and compare it with Xapi specs.

In any case if those statements are compliant with the Xapi standard, I'll be happy to review and merge any improvements included.

It's a storage-only LRS at the moment, there's no statement UI, only API where you can store and retrieve statements.

Thanks for testing!

graber → created an issue.

Merged without any testing feedback other than my own so it doesn't hang for too long.

Ok, maybe if we placed it in some folder other than .ddev, otherwise there'll be conflicts.. I can't think of a good solution at the moment though.

🙏

Sorry, it’s going to annoy me every time I run ddev commands on the platform when in the lms folder.
Please use https://github.com/graber-1/drupal_lms_ddev instead.
If someone else will be doing more work on this module they can decide wether to include it or not but as long as it’s me - no.

Please review and test https://git.drupalcode.org/project/lms/-/merge_requests/111/diffs?commit... - optimized a bit. If all fine, we can merge this.

Almost there ;)

Unfortunately als experienced this with the kickstarter repo config and also noticed that even without any changes the view config is missing things (for example try to edit the table display settings, save without any changes and export the view). I updated the view config in the kickstarter repo and in the lms_classes/config/install, hope that helps.
Not able to fix all cases of wrong config so I'm afraid it may be needed to fix the view manually after updating to 1.1.x in some cases.

I'll leave this as active so maybe there are any ideas and link the kickstarter repo config for the view: https://github.com/graber-1/drupal_lms_ddev/blob/dev/config/sync/views.v....

Just one more thing, either a regression or a pre-existing issue with the full text plugin. Should be fixed in the trait if possible so if validation fails when checking for feedback, it's visible on the screen regardless of the plugin used. More info on the MR.

Added a stub module to 1.0.x, there'll be an additional step in the upgrade - enabling it. Can't seem to do it programatically, seems like \Drupal::service('module_installer')->install(['lms_classes']); is not enough..

graber → changed the visibility of the branch 3467618-twig-error to hidden.

1 BC issue though: I couldn't figure out how to install the submodule in an update hook (anything results in missing lms_classes plugin) or even using Drush (unable to install due to existing config). Update path will be possible but tricky if there are no good ideas / existing examples, probably manually deleting the existing config and installing the module afterwards.

Tested this a bit.. without classes we have a very "vanilla" LMS where students / results view needs to be built.. Everything needed to do this (course status relationship, status and results link fields) is provided.

Will probably need additional docs in the lms_classes module and definitely a new minor bump release for lms_membership_request when this lands.

Feedback in the MR :)

graber → made their first commit to this issue’s fork.

All green.

Needs human review & hopefully a bit of testing.

Pushed some WIP.

When eventually render objects will be actually rendered, the only way to have the unprocessed form will be deep cloning of the form object so 12899++.
Instead of doing what's been done in 📌 Slowly, very slowly start OOPifying the render system Needs review , I'd start from supporting rendering of those objects in some ObjectRenderer that would be used in the current renderer conditionally if an object is passed to the render() method - creating new API next to the existing one at the very bottom of the system and only then switching gradually and not implementing some mixed array/object things in the middle of the processing chain so also ++ for reverting 📌 Slowly, very slowly start OOPifying the render system Needs review and rethinking the entire concept.

Looks good.

This will be more tricky as it'll break lms_membership_request → module. lms_membership_request → will now need 2 cases: lms_classes installed or not.
We should probably just do it without looking at lms_membership_request and update it when someone else decides to support the initiative and then update it?

I can do the work here as it doesn't look very scary but the membership request part may be a different thing.

Let's see if this will not affect automation..

graber → made their first commit to this issue’s fork.

Maybe I was a bit too fast with planning of moving the classes sub-module outside of the Drupal LMS repo as it can go out of sync at some point and will require splitting automated tests..

On the other hand.. Students don't have to be members of a course really to be able to take it with group permissions. I'll add some implementation thoughts to 📌 Considering factoring out classes to an optional module Active .

Ahh and class names can be 100% configurable as patterns with placeholder replacement (predefined or using tokens). Not a big task really.

From my knowing so far no one from the corporate world with "senior managers" on board reached out to the project maintainers or developers involved to help with their e-learning projects based on Drupal LMS. If they do, I'm sure we can work more on the project and accommodate everyone's needs.

Classes (whatever they are called) serve the purpose of separating students (whatever called) from teachers (whatever called) as it takes place in the real world education. The more a model differs from the real world, the less flexible it is and the more limitations it creates.

If needed, the classes UI can be hidden completely on a specific project and changing some translatable labels was never a big problem.

When @nicxvan mentioned ‘initializeInternalStorage’ on Slack I thought that we could save an unprocessed version that wouldn’t be a reference then. Something like ‘$this->unprocessed = $element’ (no reference) and use that when getting the unprocessed form.

Merged but this will be released in a new minor beta so it gets sufficient testing and feedback from the community before being marked as stable.

Implemented dereferencing of children in the copy MR, this fixes LMS issues.
One question: since it wasn't about the `##object` values of each element but about certain form elements being references and therefore altered as memory addresses at their source by other code.. don't we actually have a different root cause of now altering more than we altered before?
The solution works also when removed the line that unsets `$copy['##object']`: https://git.drupalcode.org/project/drupal/-/merge_requests/12901/diffs?c...
so that being there wasn't the issue.

3539320-element-plugin-object branch - fixes the LMS issue
3539320-copy branch - empty string textfield `#value` is still cached.

Ok, it doesn't solve the issue in all cases because there are other situations where a modal form should get cached before final submit (failed validation, AJAX widgets like adding field items or file uploads, ..).

I don't know if it'll solve the issue completely for LMS but not caching the form on initial build is going to be a performance improvement in any case. I'll try it now.

So far found out that for some reason on AJAX call the resulting textfield element '#value' is set to an empty string after the change which prevents FormBuilder::handleInputElement() to set its value from user input.

catch → credited graber → .

graber → created an issue.

graber → made their first commit to this issue’s fork.

Makes sense, updated.

I'd use \array_key_exists() for a more strict check but this looks good in any case, thanks!
Moving to RTBC and updating priority to critical as it results in fatal errors on sites, emails not boing sent and potentially breaking any logic that could be implemented after rendering of the messages.

graber → created an issue.

Thank you, I'm too busy with all those projects recently to handle all that, added you as a maintainer.

@catch lessons of a started course respect same rules when revisions are enabled, it's just the entity loading logic is different:
course_status -> lesson status (if exists) -> lesson revision reference

For the revisioning UI, what if the edit forms have radio buttons, with no default selection, instead of a revision checkbox?

It's one extra click in the UI vs a conscious choice. I have no idea really, we need to get more opinions on this, preferably something for later as it's something very easy to alter by project anyway.

Can we not delete all comments that explain what's going on in the code?
Also I see interface method docs are replaced by @inheritdoc. Since it's an interface that defines those methods, what is it that we inherit from?

graber → created an issue.

Thanks, that clarified a bit.

I think we're good :)

Added a draft MR, all seems to be working but still a lot of testing is needed and nav block cacheability needs to be updated.

One thing is very annoying: we need an update hook that converts the old field types to the new one, copies target_id data from the old tables and sets new data basing on current entity revisions. We can also make this feature available in the next major but still - it wouldn't be nice if we didn't provide an upgrade path.

I'm not any good with that, spend a lot of time on it already with highly unsatisfying results so.. If anyone wants to help or provide a good example from which I could just copy-paste some code, we can do it. Been there: https://www.drupal.org/docs/drupal-apis/update-api/updating-entities-and... → , https://www.drupal.org/project/field_type_converter →

If not - let's release that next major ASAP while we still don't have any or very little projects in production phase. Maintaining 2 branches will also be too much pain.

Thoughts very welcome.