Problem/Motivation

The drupal/cas_mock_server:dev-0.x branch has been created only because the main CAS module has supported, at that time, PHP 5.6. But, as now, the the lowest supported Drupal version in 8.9.x and a Drupal 8.9.x requires PHP ^7.0.8, there's no reason to keep the dependency on drupal/cas_mock_server:dev-0.x. It seems that this dependency also prevents this module to be tested on Drupal 8.9, according to #3083230: Branch 8.x-0.x should support also Drupal 8.8.x

Proposed resolution

Remove the dependency.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork cas-3207386

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

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

Status: Active » Needs review

Hm, it seems that the tests are not enabled for 2.x branch (?)

claudiu.cristea’s picture

Version: 2.x-dev » 8.x-1.x-dev

Change to branch to try to trigger the tests

claudiu.cristea’s picture

Version: 8.x-1.x-dev » 2.x-dev
StatusFileSize
new379 bytes

Let's try as a patch.

claudiu.cristea’s picture

Issue summary: View changes

Fix IS

claudiu.cristea’s picture

The failure withs PHP 7.4 & 8, from #5 shows that the module is not yet prepared for PHPUnit 8 & 9

claudiu.cristea’s picture

I can't select 2.x-dev to test. So, maybe I was wrong in #7

bkosborne’s picture

Thought I fixed the deprecations for phpunit in the 2.x branch. You were right though about tests not enabled for 2.x branch. I just enabled them. I'll take a look at this. Thank you

claudiu.cristea’s picture

StatusFileSize
new2.49 KB
new2.8 KB

@bkosborne,

The problem is that, when the deprecation has been dropped, in https://git.drupalcode.org/project/cas/commit/f75e1f7, also the method parameters should have been swapped, to follow the the externalauth method signature and also the need to avoid an optional param followed by a non optional. Now, I see that 2.0.0 is in beta. According to Drupal core standards, API changes are only allowed in ALPHA but it's up to you if you accept this change now, given that BETA is fresh and most of the sites are still on ^1.0. If this change is not acceptable, we have to deprecate the order of parameters and add message telling the they will be swapped in 3.0.0.

IMO, event is BETA, is not yet a stable release, so I would make the changes now.

bkosborne’s picture

Good catch. Yes I will make the changes in 2.x. I'll review this tonight.

Status: Needs review » Needs work

The last submitted patch, 10: 3207386-10.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new3.54 KB
new1.47 KB

I left outside one occurrence. Fixed also coding standards.

claudiu.cristea’s picture

Tests from #13 shows that everything is OK, except with Drupal 9.2.x but that is only a development branch now, not the official stable. I'll try to look into that failure.

claudiu.cristea’s picture

StatusFileSize
new7.99 KB
new11.18 KB

They've changed the password reset message in 9.2

Fixed also some coding standards. BTW, you may want to enforce coding standards by adding a drupalci.yml file. It would keep the codebase sane.

EDIT: Also, to ensure against regressions, it would be good to have weekly tests for the lowest setup (e.g. PHP7.0+SQLite+D8.9) and highest (e.g. PHP8+MySQL8+D9.2)

bkosborne’s picture

Status: Needs review » Fixed

Committed. Thank you again. I'll update the testing schedule as you suggest. That makes sense.

Status: Fixed » Closed (fixed)

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