Problem/Motivation

Some modules (BxSlider, Guardian) have their titles set with a whitespace character first. Though we could fix these modules individually, it also makes sense to trim the data and/or replace any non-letter characters at the beginning for purposes of indexing. Then the results will come back correctly regardless.

If we also remove all non-letter characters (ex.g. parentheses, @) then a module like @font-your-face will sort with the f's, and (Entity...) Something will sort with "entity."

There is also a lot of fixture related code inside the Mock plugin, which could be extracted.

Steps to reproduce

Sort A-Z for current mock data. Note that BxSlider and Guardian appear first.

Proposed resolution

(1) for the mock, remove all the non-letter leading characters (possibly) or something similar. (This would be a problem for matched parentheses, ex.g.) - alternatively, just remove whitespace.
Also, refactor the logic so functions aren't mixed up regarding the mock and the fixture generation.

(2) In the Drupal.org API site, the search index should be built to use a regex replacement of punctuation and whitespace at the beginning.

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Manual Testing
  • ☐ Code Review
  • ☐ Accessibility Review
  • ☐ Automated tests needed/written?

Issue fork project_browser-3310898

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:

About issue forks

Comments

chrisfromredfin created an issue. See original summary.

fjgarlin made their first commit to this issue’s fork.

fjgarlin opened merge request !256

fjgarlin’s picture

fjgarlin commented

While working on this I saw that the code to put the data into the database from the fixture was duplicated, so I took out one of the dupes.

fjgarlin’s picture

fjgarlin commented
Assigned: fjgarlin » Unassigned
Status: Active » Needs review

So, as part of this issue I NEEDED to work on more things:
1. Trim the whitespace (aka this issue) when saving to the database from the fixture.
2. Regenerate the fixture to test that everything would work with the above change.
3. Remove duplicated code that was in two different parts of the codebase, and move it to a place where the code was accessible on those two parts (the drush command and the .install file). The single copy of the code was moved to the .module file.

While 2 and 3 weren't part of this issue, I believe that they should still be tackled here rather than adding more logic to duplicated code. 2-related files (the fixtures) could technically be removed from this MR, but there is no damage in having an updated fixture in my opinion.

Please review.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: Unassigned » tim.plunkett

I'm looking at this more. It's a lot of code to move to fix one thing, but I agree with it

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: tim.plunkett » Unassigned

While reviewing this, I started pulling on the thread of the other fixture code that lives in MockDrupalDotOrg.
I pushed some new commits up to the branch.

If this is too much for one issue, we can split it up. But it just kept coming, until MockDrupalDotOrg was empty of anything non-runtime...

fjgarlin’s picture

fjgarlin commented
Title: Trim project titles for Mock and/or Search API D9 » Improve project fixture generation
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

If you search the codebase for Semver::satisfies you'll see two almost-identical pieces of code. Right now they're in different classes, both are moved to the same class now. But why do we have two of them? Is this MR good enough for now, to get all the wonky code in one place? Or should we keep improving it?

fjgarlin’s picture

fjgarlin commented
Status: Needs review » Reviewed & tested by the community

I went through the code and I think it's a good refactoring, making the Mock plugin a more "standard" plugin, and extracting the fixture generation into its own class/methods.

I updated the issue description to be a bit broader since we've improved the process of the fixture generation.

I then tested all in drupalpod. I re-created the whole fixture, and then got the following error:

[error]  Error: Call to undefined method Drupal\project_browser\ProjectBrowserFixture::logger() in Drupal\project_browser\ProjectBrowserFixture->hackyFixtureMaker() (line 570 of /var/www/html/repos/project_browser/src/ProjectBrowserFixture.php)

There are two lines that use $this->logger()->... but the logger service is not injected. We can either inject it or not have those two lines at all as the logging happen in the class that calls this method anyway. I went with the easiest which was removing them and tested everything and it all works now, including fixture regeneration.

@tim.plunkett, I am happy with the rest of the changes and would have marked it as RTBC if not for the above. Please re-review if you want and from my side, I'm happy with how things stand at the moment, which was more than originally planned but it's good anyway.

We can keep on improving it if we want. For example, we could do the release filtering in "getProjectReleasesFromSource" if we want to simplify the "Semver::satisfies" calls, but it's really how far we want to get at this point.

Once the Mock is gone, this class will also go away among other things that we'll need to clean up.

fjgarlin’s picture

fjgarlin commented
Status: Reviewed & tested by the community » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Agreed with @fjgarlin this is enough. It's great to clean this up, but it is destined for removal anyway.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Needs work

@fran- a quick test inside DrupalPod shows:

In YamlSymfony.php line 40:
                                                                                                                                                               
  Unexpected characters (/]) at line 6 (near "arguments: ['@logger.factory', '@project_browser.enabled_source', '@event_dispatcher', '@project_browser.fixtur  
  e_helper'/]").                                                                                                                                               
                                                                                                                                                               

In Inline.php line 285:
                                                                                                                                                               
  Unexpected characters (/]) at line 6 (near "arguments: ['@logger.factory', '@project_browser.enabled_source', '@event_dispatcher', '@project_browser.fixtur  
  e_helper'/]").

Is there a specific PHP version this might require or something like that?

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

OK, found and fixed a few errors between Tim & I with the generation but also the other drush command - the "update since fixture was generated"

Waiting for tests, then it can go.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs work » Fixed

Thanks everyone; great refactor.

Status: Fixed » Closed (fixed)

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