refactor: convert ConsentType to a PHP 8.1 backed enum#1929
Open
kayjoosten wants to merge 4 commits intofeature/stabilize-consent-hashfrom
Open
refactor: convert ConsentType to a PHP 8.1 backed enum#1929kayjoosten wants to merge 4 commits intofeature/stabilize-consent-hashfrom
kayjoosten wants to merge 4 commits intofeature/stabilize-consent-hashfrom
Conversation
When the consent feature toggle is off, upgradeAttributeHashFor was still calling _hasStoredConsent, triggering unnecessary DB queries and hash calculations. Added the same _consentEnabled early-return guard that already exists on explicitConsentWasGivenFor and implicitConsentWasGivenFor. Added a dedicated test (testUpgradeAttributeHashSkippedWhenConsentDisabled) that asserts retrieveConsentHash and updateConsentHash are never called when consent is disabled.
C1: ConsentService::countAllFor used findAllFor()+count() which loaded all consent rows into memory just to count them. Now delegates directly to consentRepository->countTotalConsent() which issues a COUNT(*) query. C2: Consent/Factory.php type-hinted the concrete ConsentHashService class instead of ConsentHashServiceInterface, violating the Dependency Inversion Principle. Changed constructor parameter, property docblock, and use import to reference the interface.
_hasStoredConsent() and _updateConsent() called sha1() on the result of _getConsentUid() without a null guard. _storeConsent() already had this guard. Added consistent guards to the remaining two methods: - _hasStoredConsent() returns ConsentVersion::notGiven() when uid is null - _updateConsent() returns false when uid is null Also fixed the ConsentTest: - Mock getNameIdValue() to return a real user ID so tests exercise the actual consent flow instead of hitting the null path - testConsentDisabledDoesNotWriteToDatabase: use shouldNotReceive() for store/retrieve/update to assert the service is never called, and add assertTrue() assertions on return values - testConsentWriteToDatabase: consolidate mock setup, add assertTrue() assertions so the test is no longer marked risky by PHPUnit
Replace the ConsentType value class (with its deprecated public constructor since ~2015) with a proper backed enum. - ConsentType is now `enum ConsentType: string` with cases Explicit = 'explicit' and Implicit = 'implicit' - The deprecated public constructor, the explicit()/implicit() named constructors and the equals() method are removed - DbalConsentRepository uses ConsentType::from() to hydrate from DB - Legacy Consent model methods now accept ConsentType and pass ->value into SQL parameter arrays - All call sites updated from TYPE_EXPLICIT/TYPE_IMPLICIT constants and named constructors to enum cases - ConsentTypeTest updated: invalid-string rejection tested via from(), equality via assertSame() on enum singletons
Contributor
Author
|
Note: @johanib ik zag hier een @deprecated van 2015.... dus ik kon het echt niet laten. Ik heb het wel apart getrokken van de ander branch maar als je dit kan reviewen merge ik hem in de andere pr. |
kayjoosten
force-pushed
the
feature/stabilize-consent-hash
branch
6 times, most recently
from
b183015 to
c3faaec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the ConsentType value class (with its deprecated public constructor since ~2015) with a proper backed enum.
enum ConsentType: stringwith cases Explicit = 'explicit' and Implicit = 'implicit'