Problem/Motivation
Since GitLab CI was enabled (#3628990, #3629003), the phpstan job reports 42 findings. phpstan only gives a warning, so the pipeline does not block, but the findings include two real bugs, deprecated API calls
that break in future versions, and many \Drupal:: calls in classes that already use, or could use, dependency injection.
Real bugs: catches that never run
bigbluebutton.install:110:catch (SqlContentEntityStorageException $e)has nousestatement, so PHP looks for a class that does not exist and the catch never runs. In the column-adding
update helper, a field without its own table stops the update instead of being skipped.BigBlueButtonHelper.php:949:catch (Exception $exception)insidenamespace Drupal\bigbluebuttonmeansDrupal\bigbluebutton\Exception, which does not exist. When creating
the BBB webhook fails with a general\Exception, the user gets an error page instead of the "Couldn't create bbb webhook" message.
Deprecated APIs
theme_get_setting()is deprecated in Drupal 11.3.0 and removed in 13.0.0.CreateMeetingParameters::addPresentation()is deprecated in bigbluebutton-api-php 3.0 and replaced byaddDocument().Feature::LAYOUTSis deprecated: layouts are no longer a valid feature since BBB 4.0.
Dependency injection
37 \Drupal:: calls in classes: 16 in BigBlueButtonHelper, 8 of them for services the class already injects, and 21 in the recording controller, the forms, the recordings formatter, the widget and the
meeting-info REST resource.
Proposed resolution
- Catches: add
use Drupal\Core\Entity\Sql\SqlContentEntityStorageException;tobigbluebutton.install, and changecatch (Exception …)to
catch (\Exception …).
theme_get_setting(): injectDrupal\Core\Extension\ThemeSettingsProvideras an optional service (@?). On Drupal 10 and 11.0–11.2 the service does not exist,
andgetLogoUrl()falls back totheme_get_setting(). The fallback can be removed when Drupal 10 support is dropped.addPresentation(): useaddDocument(new DocumentUrl($file_url)). Without a file name, the XML sent to BBB is byte-identical toaddPresentation(), so nothing changes for
any BBB server version.Feature::LAYOUTS: compare by value ($feature->value === 'layouts'). "Layouts" stays hidden from the disabled-features options, as before.- Dependency injection:
BigBlueButtonHelper: use the services that are already injected (token,file_url_generator,entity.repository,request_stack), and inject
cache.default,datetime.time,http_clientanddate.formatter. The new arguments go after the existing ones.BigBlueButtonRecordingController: inject the helper throughcreate(), and useControllerBase::config().BBBDeleteRecordingForm: useFormBase::config().BBBLinkForm:bigbluebutton.dynamic_form_factorynow passes the helper and the entity type manager into the form. Remove the unused$user = \Drupal::currentUser().SettingsForm:create()throughparent::create(), so it works with theConfigFormBaseconstructor of every supported core version.BigBlueButtonRecordingsFormatter:create()with the helper, config factory, current user and form builder.BigBlueButtonDefaultWidget: inject the helper.BigBlueButtonGetMeetingInfo: use the helper that is already injected.
No behaviour changes are intended. Nothing sent to the BBB server changes.
Remaining tasks
- Commit, and confirm that the
phpstanjob reports no findings forsrc/andbigbluebutton.install, and thatphpunitis green on Drupal 10 and 11. - Follow-up (optional): make the
phpstanjob blocking once it is clean. - Follow-up (optional): offer "layouts" as a disabled feature again for BBB servers older than 4.0.
User interface changes
None.
API changes
Constructors gain arguments: BigBlueButtonHelper (5 new arguments at the end, with the last one optional), BBBLinkForm (helper and entity type manager; $settings is now required),
BigBlueButtonDynamicFormFactory, BigBlueButtonRecordingController and BigBlueButtonDefaultWidget. Inside the module these are only created through the container, create() or the
form factory. Code that creates them directly with new, or extends BBBLinkForm, BigBlueButtonRecordingController or BigBlueButtonDefaultWidget and calls their constructor, must
be updated.
Data model changes
None.
Comments