Problem/Motivation
1.0.0-alpha6 moved the API client, its event and exception to the base module (#3629746: Move the Graph API client to the base module and add a typed API exception) and the image importer to mapillary_entity (#3630051: Fix the remaining module review findings: API calls on save, endless queue retries, viewer collisions and standalone submodules), and changed several constructors. The old names and signatures were kept with deprecations, so that code using them had a release to move to the new ones. The compatibility layer is 17 items in seven groups:
- Old class files in
mapillary_field/src/, which only trigger a deprecation and load the new class:Service/MapillaryApiClient.php,Service/MapillaryImageImporter.php,Event/MapillaryApiCallEvent.phpandException/MissingAccessTokenException.php. class_alias()calls at the bottom of the new class files, forcatchblocks and type hints that do not trigger the autoloader:Drupal\mapillary\Service\MapillaryApiClient,Drupal\mapillary\Event\MapillaryApiCallEvent,Drupal\mapillary\Exception\MissingAccessTokenExceptionandDrupal\mapillary_entity\Service\MapillaryImageImporter.- Deprecated service aliases in
mapillary_field.services.yml:mapillary_field.api_clientandmapillary_field.image_importer. - Old constructor signatures: a logger as the third argument of
MapillaryApiClient, and a messenger as the third argument ofMapillaryImageImporter, the fourth ofMapillaryMapFeatureImporterand the fifth ofMapillaryDetectionImporter, each followed by a$legacyLoggeror event dispatcher argument. MapillaryImageImporter::importSequence()with$cascadeImagesset toTRUE, its default, which imports every image of the sequence in one request.MapillaryThumbnailRefresher::isServiceWideFailure(), replaced byMapillaryApiException::isServiceWide().MapillaryLeafletWidgetBase::GRAPH_BASE_URL, replaced byMapillaryBrowserSettings::GRAPH_BASE_URL. Its deprecation is only in the docblock; the 1.0.0-alpha6 changelog does not list it.
The tests also create mock objects that never get an expectation. PHPUnit 12 reports each of them as a notice: No expectations were configured for the mock object for ... Consider refactoring your test code to use a test stub instead. A run on Drupal 12.0.0-beta1 with PHPUnit 12.5 reported 289 such notices in 22 test classes. One of them, LegacyApiClassNamesTest, goes away with the compatibility layer; the other 21:
mapillary:MapillaryApiClientTest(26 notices),HelpHooksTest,ApiQueueFailureTraitTest.mapillary_entity:MapillaryImageImporterTest(14),ImageImporterTest.mapillary_field:MapillaryFieldHooksTest(10),MapillaryThumbnailRefresherTest(10),MapillarySequenceCascadeQueuerTest,MapillarySequenceImageImportWorkerTest,MapillaryThumbnailRefreshWorkerTest,MapillaryImportFormsTest,MapillarySequenceIdFormatterTest,SequenceCascadeQueueTest.mapillary_logs:MapillaryLogsServiceTest(12),MapillaryLogsHooksTest(8).mapillary_leaflet:MapillaryLeafletWidgetTest,MapillaryLeafletSequenceWidgetTest.mapillary_detection:MapillarySequenceDetectionImportWorkerTest,MapillarySequenceDetectionCascadeQueuerTest,DetectionImporterTest.mapillary_map_feature:MapFeatureImporterTest.
Steps to reproduce
- Search the module for
deprecated in mapillary:1.0.0-alpha6. - Run the tests of the module with PHPUnit 12, for example on Drupal 12, and read the PHPUnit notices at the end of each test class.
Proposed resolution
- Delete the four old class files in
mapillary_field/src/and the fourclass_alias()calls with their@todocomments. - Delete the two service aliases from
mapillary_field.services.yml. - Give the four constructors only their current signature: the logger or event dispatcher as a typed argument, no union type, no
$legacyLogger, noInvalidArgumentExceptionfor the old argument order. - Remove
importSequence(). Without the cascade it only callsimportSequenceMetadata()and returns the entity, and keeping it without the parameter would change what existing callers get without an error:importSequence($id)andimportSequence($id, TRUE)would return a sequence without images, since PHP ignores the extra argument. Callers useimportSequenceMetadata($id)->entityand queue the images withmapillary_field.sequence_cascade_queuer. The module itself calls it only in tests. - Remove
isServiceWideFailure()and theGRAPH_BASE_URLconstant of the widget base. - Remove the tests of the compatibility layer:
LegacyApiClassNamesTest,LegacyApiClientServiceTestandMapillaryThumbnailRefresherFailureTest;testLegacyConstructorWithLogger()inMapillaryApiClientTest;testImportSequenceWithCascadeIsDeprecated()andtestOldServiceIdAndClassName()in the kernelImageImporterTest;testImportSequenceCascadeUsesRealPkForImages()andtestLegacyConstructorSignature()in the unitMapillaryImageImporterTest. The unit tests that callimportSequence($id, FALSE)callimportSequenceMetadata()instead. - Update
AGENTS.md, which describes the compatibility layer for the API client and the image importer. - Add a "Removed" section to
CHANGELOG.mdwith every item above, the widget constant included. - The release notes ask sites on 1.0.0-alpha5 or older to update to 1.0.0-alpha6 and run the database updates first. Their cached container still builds
mapillary_field.api_clientfrom a class that no longer exists, and the importers with the old arguments. A site on 1.0.0-alpha6 already uses the new service definitions, so the update needs no update hook. - After the removals, in the remaining tests, create a test double that only returns values with
createStub()instead ofcreateMock(). Where the test should check a call, keep the mock and addexpects().#[AllowMockObjectsWithoutExpectations]is not used: it hides the notice without saying what the test checks.createStub()is available in PHPUnit 11, so the tests keep running on Drupal 11. - Run the test suite on Drupal 11 and Drupal 12, with no PHPUnit notices left on Drupal 12.
Remaining tasks
- Merge request.
- Review.
- Update rehearsal from 1.0.0-alpha6.
User interface changes
- None.
API changes
- Removed classes:
Drupal\mapillary_field\Service\MapillaryApiClient,Drupal\mapillary_field\Service\MapillaryImageImporter,Drupal\mapillary_field\Event\MapillaryApiCallEventandDrupal\mapillary_field\Exception\MissingAccessTokenException. - Removed services:
mapillary_field.api_clientandmapillary_field.image_importer. - Removed methods:
MapillaryImageImporter::importSequence()andMapillaryThumbnailRefresher::isServiceWideFailure(). - Removed constant:
MapillaryLeafletWidgetBase::GRAPH_BASE_URL. - Constructors of
MapillaryApiClient,MapillaryImageImporter,MapillaryMapFeatureImporterandMapillaryDetectionImporteraccept only their current arguments. - The event name
mapillary_field.api_callis not deprecated and stays. - The test changes do not change any API.
Data model changes
- None.
Issue fork mapillary-3631465
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #4
orkutmuratyilmaz