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.php and Exception/MissingAccessTokenException.php.
  • class_alias() calls at the bottom of the new class files, for catch blocks and type hints that do not trigger the autoloader: Drupal\mapillary\Service\MapillaryApiClient, Drupal\mapillary\Event\MapillaryApiCallEvent, Drupal\mapillary\Exception\MissingAccessTokenException and Drupal\mapillary_entity\Service\MapillaryImageImporter.
  • Deprecated service aliases in mapillary_field.services.yml: mapillary_field.api_client and mapillary_field.image_importer.
  • Old constructor signatures: a logger as the third argument of MapillaryApiClient, and a messenger as the third argument of MapillaryImageImporter, the fourth of MapillaryMapFeatureImporter and the fifth of MapillaryDetectionImporter, each followed by a $legacyLogger or event dispatcher argument.
  • MapillaryImageImporter::importSequence() with $cascadeImages set to TRUE, its default, which imports every image of the sequence in one request.
  • MapillaryThumbnailRefresher::isServiceWideFailure(), replaced by MapillaryApiException::isServiceWide().
  • MapillaryLeafletWidgetBase::GRAPH_BASE_URL, replaced by MapillaryBrowserSettings::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

  1. Search the module for deprecated in mapillary:1.0.0-alpha6.
  2. 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 four class_alias() calls with their @todo comments.
  • 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, no InvalidArgumentException for the old argument order.
  • Remove importSequence(). Without the cascade it only calls importSequenceMetadata() and returns the entity, and keeping it without the parameter would change what existing callers get without an error: importSequence($id) and importSequence($id, TRUE) would return a sequence without images, since PHP ignores the extra argument. Callers use importSequenceMetadata($id)->entity and queue the images with mapillary_field.sequence_cascade_queuer. The module itself calls it only in tests.
  • Remove isServiceWideFailure() and the GRAPH_BASE_URL constant of the widget base.
  • Remove the tests of the compatibility layer: LegacyApiClassNamesTest, LegacyApiClientServiceTest and MapillaryThumbnailRefresherFailureTest; testLegacyConstructorWithLogger() in MapillaryApiClientTest; testImportSequenceWithCascadeIsDeprecated() and testOldServiceIdAndClassName() in the kernel ImageImporterTest; testImportSequenceCascadeUsesRealPkForImages() and testLegacyConstructorSignature() in the unit MapillaryImageImporterTest. The unit tests that call importSequence($id, FALSE) call importSequenceMetadata() instead.
  • Update AGENTS.md, which describes the compatibility layer for the API client and the image importer.
  • Add a "Removed" section to CHANGELOG.md with 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_client from 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 of createMock(). Where the test should check a call, keep the mock and add expects(). #[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\MapillaryApiCallEvent and Drupal\mapillary_field\Exception\MissingAccessTokenException.
  • Removed services: mapillary_field.api_client and mapillary_field.image_importer.
  • Removed methods: MapillaryImageImporter::importSequence() and MapillaryThumbnailRefresher::isServiceWideFailure().
  • Removed constant: MapillaryLeafletWidgetBase::GRAPH_BASE_URL.
  • Constructors of MapillaryApiClient, MapillaryImageImporter, MapillaryMapFeatureImporter and MapillaryDetectionImporter accept only their current arguments.
  • The event name mapillary_field.api_call is not deprecated and stays.
  • The test changes do not change any API.

Data model changes

  • None.

Issue fork mapillary-3631465

Command icon 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

orkutmuratyilmaz created an issue. See original summary.

  • 2be51877 committed on 1.0.x
    bug: #3631465: Remove the code deprecated in Mapillary Module
    
    Remove...
orkutmuratyilmaz’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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