Account created on 26 February 2013, over 12 years ago
#

Merge Requests

More

Recent comments

🇪🇸Spain fjgarlin

There is an attempt at removing orphan entities: https://git.drupalcode.org/project/api/-/blob/2.x/api.module?ref_type=he...

It's probably not configured. So we should try this first.

🇪🇸Spain fjgarlin

That's not the way the templates work. Evertyhing is documented in https://project.pages.drupalcode.org/gitlab_templates/jobs/, in the "When do the jobs run?" section.

What you need (as mentioned in the slack there https://drupal.slack.com/archives/C08CJ9K74MB/p1749769260020559) is the following:

composer:
  variables:
    DRUPAL_CORE: 10.2.0
🇪🇸Spain fjgarlin

cspell cannot work with symlinked files

That might have been the reason why the approach on this job was different to the others.

100% agree with #15 about reverting and trying the other approach (and 100% on the grateful-but-no-pressure part too).

🇪🇸Spain fjgarlin

Looks good, and totally fine to leave that debug flag for when we run the downstream pipelines too.

🇪🇸Spain fjgarlin

You can have the queue job running every 10 minutes (for example) and use the --time-limit=590 parameter for a new clean run. That way the queue will be continuously running but should not take too much memory. Change 10 minutes for whatever your memory threshold / time is.

🇪🇸Spain fjgarlin

Falta el video. Una vez este subido podemos cerrar el issue.

Gracias @jlbellido!

🇪🇸Spain fjgarlin

I added a bit to the message, but the whole logic is perfect in my opinion.

I don't think we need to worry about the allow_failure setting. If it's false, this addition will make the pipeline fail, and rightfully so, because they want the phpstan job and this is not possible, so they need to do something about it.

RTBC after the suggestion is applied (feel free to change the language if needed) and the testing code is removed. NW for now.

🇪🇸Spain fjgarlin

That is a great point. We need to test and check what's going to happen there.

I added it as a blocker, together with another important case that will need to happen as well.

🇪🇸Spain fjgarlin

I guess it makes sense to do the cd $DRUPAL_PROJECT_FOLDER to match what the other jobs do. We might have just forgotten about cspell when we did the change.

NW based on your suggestions.

🇪🇸Spain fjgarlin

Hi @vinojbi and @vinodhini.e. Note that the issue was already RTBC in #6 the day before you commented on this issue. Also the screenshots provided don't add anything new, as there was one already in #4 with the same changes.

@vinodhini.e, adding your company to both organization and customer won't have any double impact on credits. In fact, doing so might be seen as wanting to game the contribution system to try to obtain double credit.

I recommend you both to go through this page https://www.drupal.org/community/contributor-guide , see the links in it (specially "Issue Queue Etiquette") and even the two videos at the bottom. These resources will help you improve the quality of your contributions, making sure that the time that you spend is meaningful and impactul for you and the community.

🇪🇸Spain fjgarlin

Code looks good and really simple. We are also replacing the Printer as it was intended when the separate repo was created. Tests are happy so I'm merging this.

🇪🇸Spain fjgarlin

Code looks good and the tests are still happy (including the new additions).

🇪🇸Spain fjgarlin

I just checked this again and I think that the current implementation is correct.

The header and the footer are outside the main content area. The main menu is outside, but visually is on top of the content, and the content has padding on top to accommodate for this.

I'm going to mark it as "Fixed" again as I think this is what is expected.

🇪🇸Spain fjgarlin

Yeah, the output from run-tests.sh is the URL given in SIMPLETEST_BASE_URL, which is http://localhost/web in most cases (this line).

The tests can be run locally, on GitLab CI, on a jenkins server (as before) or any other system, so we cannot give that script the final GitLab pages URL (eg: https://project.pages.drupalcode.org/-/keycdn/-/jobs/5521834/artifacts/).

Having said that, something that we could try is to run the run-tests.sh command and capture the output command > output.log, and then run a sed command on that output to replace http://localhost/ with https://project.pages.drupalcode.org/-/$CI_PROJECT_NAME/-/jobs/$CI_JOB_ID/artifacts/ and then show that file as output.

The question about this approach is whether we will get real-time feedback, as we do now vs needing to wait for the full run and then dump it all in one go. This wouldn't be a good user experience in my opinion.

We can try doing it and see how far we get or we could even make a variable to get this behaviour (or use PHPUNIT_EXTRA to pass that option).

🇪🇸Spain fjgarlin

I just moved the file inside a scripts folder. It looks good. Merged.

🇪🇸Spain fjgarlin

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

🇪🇸Spain fjgarlin

The real problem here is this

$ php $CI_PROJECT_DIR/vendor/bin/phpstan --version
Could not open input file: /builds/project/seven/vendor/bin/phpstan

phpstan comes from drupal/core-dev, and this was only included in versions 10+, not 9 or below.

The fix here should be to check for the phpstan binary directly, if not present, exit with a warning before going any further (not an error because we haven't actually checked the code). Let's make sure that the warning is visible via the $DIVIDER syntax that we've used in other places.

I don't think that we need to do if/else logic about Drupal versions, or try to bring phpstan in any other way. I would just check for the existence of the tool and bail out early if not present with an useful message.

🇪🇸Spain fjgarlin

I think the$DRUPAL_PROJECT_FOLDER was just introduced later than the latest changes we did in cspell job.

We can either add the internal file to our ignore list for cspell or just try to run cspell on the $DRUPAL_PROJECT_FOLDER folder directly.

🇪🇸Spain fjgarlin

This is a great refactoring! RTBC.

Will try to test further locally later too.

🇪🇸Spain fjgarlin

I guess it's PerimeterX blocking this @drumm

🇪🇸Spain fjgarlin

MR19 and MR20 look good.

For modern Drupal, we have the composer output, but yeah, maybe a 1-2 liner with just the contrib modules can be nice (separate issue).

🇪🇸Spain fjgarlin

I thought that it was intentional in this MR, that's why I didn't mention anything. Agree, we can review that code in the related issue.

🇪🇸Spain fjgarlin

Yeah, GitLab generates random unique domain names, but they are not predictable. It's nicer as a path from projects.pages.drupalcode.org in my opinion.

🇪🇸Spain fjgarlin

Merged. Thanks for noticing the documentation page!

🇪🇸Spain fjgarlin

The MR looks good. RTBC.

I think there is a setting in the Gitlab repo admin area to fix the URL, if that's what we want here.
It's under Deploy > Pages > Set unique domain name (it might be checked by default and we just need to uncheck it).

🇪🇸Spain fjgarlin

Hopefully this will help (if not fix) with the related issues.

🇪🇸Spain fjgarlin

Code looks good. It's got test coverage. RTBC and will merge with other RTBC issues later today.

🇪🇸Spain fjgarlin

Code looks good and there are supporting tests. Will merge soon with other RTBC issues.

🇪🇸Spain fjgarlin

Code looks good. It's got test coverage. RTBC and will merge with other RTBC issues later today.

🇪🇸Spain fjgarlin

It looks good to me. Thanks for the testing in all those cases.

RTBC pending the revert mentioned in the MR.

🇪🇸Spain fjgarlin

That rule is specified here: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

It makes sense to run an extra round of CI on tags for most projects. If your project doesn't need this you can always override the default rules that we provide.

🇪🇸Spain fjgarlin

The icon file is still empty in the fork.

🇪🇸Spain fjgarlin

Looks good. Merging shortly.

🇪🇸Spain fjgarlin

More edge cases:
- Issues with no title
- Issues with no comments and the users who created them no longer have an account

MR for that: https://git.drupalcode.org/project/drupalorg/-/merge_requests/355/diffs

🇪🇸Spain fjgarlin

Yeah, agree. We can have them all under the same variable.

🇪🇸Spain fjgarlin

Yeah, that sounds like a good way forward. We can/should unify the extentions being checked for D8+ and add the extra ones for D7.

🇪🇸Spain fjgarlin

I feel like we've been through this in the past already. Nobody is forcing you to use the tooling that we provide, that part is really opt-in.

Also, you know that you can pin the version of the templates that you want/need, and then you won't get any changes: https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver...

So, if you choose not to do that, then you are choosing to stay up to date and receive the "defaults", which will change over time as core moves forward. Some people like this way, some people don't, and that's why you can choose.

🇪🇸Spain fjgarlin

After the first full import run, we've had around ~1% missing on the new site. The reason for most of these (if not all) was that the issue didn't even have a comment, not even the automated comment #1 which is always linked to the author.

We will send the author with no attribution (as none can be calculated) when there are no comments on the issue. If the author makes a comment with attribution, then this last one will be taken instead.

MR to fix this situation: https://git.drupalcode.org/project/drupalorg/-/merge_requests/353/diffs

After that, we will need to re-run some of the above commands with selected NIDs.

🇪🇸Spain fjgarlin

Yeah, but it's not under the same path, that's the issue.

We could defo improve the matching, trying to get the object being seen and see if there is the same object in the latest version. Or even explode the URL and try to guess it. eg: https://api.drupal.org/api/drupal/core%21modules%21system%21system.token... would be "system_token_info", and search would find that.

🇪🇸Spain fjgarlin

The alternative was either to go to the homepage of the supported/preferred branch or to trigger the search.

🇪🇸Spain fjgarlin

It's not really broken. If it can't find the equivalent page in the latest version, it triggers a search for the path.

See the message:

Sorry, the path ... cannot be matched against any page in this branch. Try searching by one of the components of the path.

The main issue that I see is that the status message is not styled. The classes seem to be ok, so it's the CSS somehow not loading.

🇪🇸Spain fjgarlin

Not sure what can be done from here. We parse information that already exist in the code. If the comments of the code needs improving then we should file a core issue.

🇪🇸Spain fjgarlin

MR can remain active if needed.

🇪🇸Spain fjgarlin

Repurposing the issue to remove all comments logic as we no longer use the feature and we don't need to maintain the code around it.

🇪🇸Spain fjgarlin

I no longer see that happening.

🇪🇸Spain fjgarlin

We reduced the number of branches and we sort them by weight set in the database.

🇪🇸Spain fjgarlin

We have a "parsed" attribute in the branch that any object belongs to. We could show that.

🇪🇸Spain fjgarlin

Not sure if still applicable. We can check in the new code.

🇪🇸Spain fjgarlin

I'm considering that we can remove links to php.net, the same way that we stopped parsing the vendor folders (therefore calls to symfony objects are never linked nor parsed).

We could repurpose this issue to clean up all logic for "external branches" as the only use case we have is PHP and it is no longer working (nor probably needed).

🇪🇸Spain fjgarlin

http://doc.php.net/downloads/json/php_manual_en.json is no longer available. We have an issue for that.

This issue might be too much work to link to known PHP code (via php.net). We removed vendor folders a while back, so probably we can even remove all PHP function calls.

I'm going to close the issue for now but feel free to discuss further if needed.

🇪🇸Spain fjgarlin

Closing old issues as we are now on modern Drupal and the site has changed since the issues were created. Re-open/comment if needed.

🇪🇸Spain fjgarlin

Closing old issues as we are now on modern Drupal and the site has changed since the issues were created. Re-open/comment if needed.

🇪🇸Spain fjgarlin

Closing old issues as we are now on modern Drupal and the site has changed since the issues were created. Re-open/comment if needed.

🇪🇸Spain fjgarlin

Core never implemented this. Closing the issue.

🇪🇸Spain fjgarlin

Merged. Thanks for the improvement to the script and log.

🇪🇸Spain fjgarlin

Agree. Easier to read and no need to make assumptions, just fail if not all inputs needed are present.

🇪🇸Spain fjgarlin

The solution looks good.

I think it's ready for final clean up and RTBC. NW for now until the clean up happens.

Production build 0.71.5 2024