The ckeditor module uses test class members with underscored names. Some examples are big_user, web_user and admin_user, but there could be others. According to our coding conventions, these should be renamed to bigUser, webUser and adminUser. In addition, some properties are undefined but should be.

See the parent issue #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because this is a coding standards change.
Issue priority Not critical because coding standard changes are not critical.
Unfrozen changes Unfrozen because it only changes automated tests.
Disruption There is no disruption expected from this sort of change.
CommentFileSizeAuthor
#17 2381303-17.patch4.3 KBrpayanm
#17 2381303-interdiff.txt811 bytesrpayanm
#15 clean_up_ckeditor-2381303-interdiff-5-15.txt1.39 KBtibbsa
#15 clean_up_ckeditor-2381303-15.patch4.26 KBtibbsa
#5 clean_up_ckeditor-2381303-5.patch3.41 KBhussainweb
#5 interdiff-2-5.txt2.11 KBhussainweb
#2 clean_up_ckeditor-2381303-2.patch1.3 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Clean-up ckeditor module test members - ensure property definition and use of camelCase naming convention » Clean-up CKEditor module test members — ensure property definition and use of camelCase naming convention
Priority: Normal » Minor
Issue tags: +php-novice

Yep, those property names with underscores are from the previous test coding standard conventions.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented
Status: Active » Needs review
FileSize
clean_up_ckeditor-2381303-2.patch1.3 KB

Just found references to admin_user, cleaned up.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php contains untrusted_user and normal_user.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

@mglaman The parent issue summary has a grep command that may help you find any other missed instances.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review
FileSize
interdiff-2-5.txt2.11 KB
clean_up_ckeditor-2381303-5.patch3.41 KB

I checked with the grep command and found no other files in ckeditor module.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

tibbsa’s picture

tibbsa CreditAttribution: tibbsa commented
Status: Reviewed & tested by the community » Needs work

In CKEditorLoadingTest:

  • Class property untrustedUser implicitly created without prior declaration
  • Class property normalUser implicitly created without prior declaration

These should be provided with declarations and docblocks.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented
Status: Needs work » Reviewed & tested by the community

Moving back to RTBC. tibbsa, see parent meta issue

In addition, some properties are undefined but should be (see #2380023-4: Clean-up Comment module Test members - ensure property definition and use of camelCase naming convention).

The declarations and docblock changes are outside of the scope for this meta issue.

tibbsa’s picture

tibbsa CreditAttribution: tibbsa commented

Exactly how is "ensure property definition" (perhaps this should say 'declaration') outside the scope of this issue?

This was the comment and first instance which originally resulted in that note being added to the parent issue and the scope slightly expanded: #2380773-1: Clean-up Node module test members - ensure property definition and use of camelCase naming convention

While adding docblocks to random variables may not be "in scope", adding new property declarations in the class definition (which implies documenting them in the process) is and those changes have been made in all the other sub-issues. That is what is meant by "ensure property definition".

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Needs work

There are more to be done.

$ egrep -r '\$this\->[a-z]+_[a-z_]+' core/modules/ckeditor/
core/modules/ckeditor//src/Tests/CKEditorAdminTest.php:    $this->admin_user = $this->drupalCreateUser(array('administer filters'));
core/modules/ckeditor//src/Tests/CKEditorAdminTest.php:    $this->drupalLogin($this->admin_user);
core/modules/ckeditor//src/Tests/CKEditorAdminTest.php:    $this->drupalLogin($this->admin_user);
core/modules/ckeditor//src/Tests/CKEditorLoadingTest.php:    $this->untrusted_user = $this->drupalCreateUser(array('create article content', 'edit any article content'));
core/modules/ckeditor//src/Tests/CKEditorLoadingTest.php:    $this->normal_user = $this->drupalCreateUser(array('create article content', 'edit any article content', 'use text format filtered_html', 'use text format full_html'));
core/modules/ckeditor//src/Tests/CKEditorLoadingTest.php:    $this->drupalLogin($this->untrusted_user);
core/modules/ckeditor//src/Tests/CKEditorLoadingTest.php:    $this->drupalLogin($this->normal_user);

Also, the purpose of the meta is to declare the test properties. The Drupal coding standards imply they must also be documented, so let's do that please.

thomasdegraaff’s picture

thomasdegraaff CreditAttribution: thomasdegraaff commented

I'm going to fix the issues mentioned in #10.

Edit:
The code changes mentioned in #10 are already fixed in patch #5

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Reviewed & tested by the community

I agree with @Thomas de Graaff, then RTBC

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Needs work

@Thomas de Graaff , sorry, I must have reviewed the wrong patch.

adminUser, untrustedUser, and normalUser are not defined.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Agreed this should follow the standard set in #2384165: Clean-up Contextual module test members — ensure property definition and use of camelCase naming convention and explicitly declare these new member variables, w/ docs. We do not want to be introducing more code to clean up later, especially in a minor task that's not related to getting D8 released faster in any way.

tibbsa’s picture

tibbsa CreditAttribution: tibbsa commented
Status: Needs work » Needs review
FileSize
clean_up_ckeditor-2381303-15.patch4.26 KB
clean_up_ckeditor-2381303-interdiff-5-15.txt1.39 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

#15: Thanks, looks great!

But now that it's documented there, let's remove this:

    // Create 2 users, each with access to different text formats:
    //   - "untrusted": plain_text
    //   - "normal": plain_text, filtered_html
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2381303-interdiff.txt811 bytes
2381303-17.patch4.3 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed ca3cc9b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed ca3cc9b on 8.0.x 
    Issue #2381303 by tibbsa, rpayanm, hussainweb, mglaman: Clean-up...

Status: Fixed » Closed (fixed)

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