Skip to content

Conversation

ritankarsaha
Copy link
Contributor

This PR fixes the issue #2833

Description

To implement this issue I have added graceful fallback and null-checks

  • Adding Null Checks - Before invoking any method on a license object, the code now checks whether the object is null. If it is, the code either skips processing that record or logs a warning.
  • For critical cases if the license is not found, the code creates a fallback license object with default values.

This ensures that subsequent methods liked getId() or getText() can be made safely on the fallback object and the report generation continues without a fatal error.

Changes

  • Null-Check in LicenseMainGetter.php:
    Added a check for null after retrieving the license object. If it’s missing, the code logs a warning and skips processing that license instead of calling methods on a null object.

  • Fallback License in spdx.php:
    Added a null-check , If it isn’t found, a fallback License object is created with default values so that subsequent method calls (like getId() and getText()) succeed.

Testing

  • Simulate a Missing License:
  • Run a Test Upload:
  • Monitor the Logs:
  • Check your Fossology logs (e.g., with tail -f /var/log/fossology/fossology.log) for a warning message
  • Review the Generated Report:

$filesProceeded += 1;
if (($filesProceeded&2047)==0) {
$this->heartbeat(0);
$this->heartbeat($filesProceeded - 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ritankarsaha Can you explain why this change is required?

Copy link
Contributor Author

@ritankarsaha ritankarsaha Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @its-sushant, thanks for the review! with this change I am only passing the actual number of processed files instead of always sending 0 so that the agent can give more detailed feedback on it's work. Like for example if there were 100 files, and a job fails at the 20th file, then the user can see that it has failed at the 20th file and how much work had been done before it failed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ritankarsaha : for now this change is not needed can you please remove tis so that we can merged this branch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @shaheemazmalmmd will make the changes and push the new commit !

@ritankarsaha ritankarsaha force-pushed the ritankarsaha/fix/2833/phpfatalerror branch 2 times, most recently from 9b29f7e to cf27556 Compare April 18, 2025 20:42
@ritankarsaha
Copy link
Contributor Author

@shaheemazmalmmd done with the changes! fixed the code sniffer error, signed the commits, squashed them into one.

@its-sushant
Copy link
Contributor

image
@ritankarsaha I tried to generate DEP, ReadmeOSS and spdx report after deleting main license from the database. Working fine for ReadmeOSS but still trowing same error for DEP and spdx report.

@ritankarsaha
Copy link
Contributor Author

image @ritankarsaha I tried to generate DEP, ReadmeOSS and spdx report after deleting main license from the database. Working fine for ReadmeOSS but still trowing same error for DEP and spdx report.

Oh, I am looking into it and providing with a fix ASAP @its-sushant !

@ritankarsaha ritankarsaha force-pushed the ritankarsaha/fix/2833/phpfatalerror branch from cf27556 to a5281e7 Compare April 26, 2025 09:39
@ritankarsaha
Copy link
Contributor Author

image @ritankarsaha I tried to generate DEP, ReadmeOSS and spdx report after deleting main license from the database. Working fine for ReadmeOSS but still trowing same error for DEP and spdx report.

Hello, @its-sushant I have pushed a new commit into the PR. I hope this aligns with the requirements now. Are there any other modifications that are needed to be made?

@ritankarsaha
Copy link
Contributor Author

Hey @its-sushant I was wondering if there is any command to fix the indentation errors and formatting errors all at once rather than fixing them manually?

@ritankarsaha ritankarsaha force-pushed the ritankarsaha/fix/2833/phpfatalerror branch 3 times, most recently from 1db8969 to 9f05150 Compare April 28, 2025 06:05
@ritankarsaha
Copy link
Contributor Author

Hey @its-sushant fixed the codesniffer errors and pushed the new commit !

Copy link
Contributor

@its-sushant its-sushant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ritankarsaha Thank you for incorporating with the request. I have added few more suggestions. Please take a look!

Comment on lines 228 to 231
if ($docLicense === null) {
error_log("Warning: License with short name " . self::DATA_LICENSE . " not found in the database. Using fallback license.");
$docLicense = new License(0, self::DATA_LICENSE, "Fallback License", "Unknown", "No license text available", "", "", "");
}
Copy link
Contributor

@its-sushant its-sushant Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need of this check. Also ideally we should not create any Fallback License for report and we can always expect the DATA_LICENSE exist in database.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@its-sushant removed this check !!

Comment on lines 894 to 906
if ($dataLic === null) {
error_log(
"spdx: warning: data‐license '" . self::DATA_LICENSE . "' not found; using fallback."
);
$dataLic = new \Fossology\Lib\Data\License(
0,
self::DATA_LICENSE,
"Fallback Data License",
"Unknown",
"No license text available",
"", "", ""
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check can also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@its-sushant also removed this check !!

@ritankarsaha
Copy link
Contributor Author

@ritankarsaha Thank you for incorporating with the request. I have added few more suggestions. Please take a look!

Yess @its-sushant updating them accordingly and pushing a new commit !!

@ritankarsaha ritankarsaha force-pushed the ritankarsaha/fix/2833/phpfatalerror branch from 9f05150 to 74b890c Compare April 30, 2025 23:21
@ritankarsaha
Copy link
Contributor Author

Hey @its-sushant removed the blocks, and pushed a new commit into the PR. :))

Copy link
Contributor

@its-sushant its-sushant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ritankarsaha We can simply fail the agent when main license is not found in Database. I have added few suggestions. Hopefully this will be last 😄

Comment on lines 44 to 45
error_log("Warning: License ID " . $originLicenseId . " not found in the database.");
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
error_log("Warning: License ID " . $originLicenseId . " not found in the database.");
continue;
error_log("Error: License ID " . $originLicenseId . " not found in the database.");
exit;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this @its-sushant !

Comment on lines 386 to 388
"spdx: warning: main license ID {$reportedLicenseId} not found; skipping."
);
continue;
Copy link
Contributor

@its-sushant its-sushant May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"spdx: warning: main license ID {$reportedLicenseId} not found; skipping."
);
continue;
"spdx: Error: main license ID {$reportedLicenseId} not found."
);
exit;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this as well @its-sushant

@ritankarsaha
Copy link
Contributor Author

@ritankarsaha We can simply fail the agent when main license is not found in Database. I have added few suggestions. Hopefully this will be last 😄

Sure @its-sushant will make these modifications and add a new commit !!

@ritankarsaha ritankarsaha force-pushed the ritankarsaha/fix/2833/phpfatalerror branch from 74b890c to d772683 Compare May 2, 2025 19:27
@ritankarsaha
Copy link
Contributor Author

@ritankarsaha We can simply fail the agent when main license is not found in Database. I have added few suggestions. Hopefully this will be last 😄

Hey @its-sushant I have made the necessary updates and pushed the new commit :)). Also apologies for the delay, I actually have my end-sems going !

@ritankarsaha
Copy link
Contributor Author

@ritankarsaha We can simply fail the agent when main license is not found in Database. I have added few suggestions. Hopefully this will be last 😄

Hello @its-sushant is the implementation of the PR now alright and in accordance or are there some other modifications that I need to make as well to this ?

Copy link
Contributor

@its-sushant its-sushant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. LGTM! Thank you for the contribution @ritankarsaha

Cc: @shaheemazmalmmd @Kaushl2208

@ritankarsaha
Copy link
Contributor Author

Tested. LGTM! Thank you for the contribution @ritankarsaha

Cc: @shaheemazmalmmd @Kaushl2208

Thank You !! @its-sushant 🙃

@ritankarsaha
Copy link
Contributor Author

Hey @its-sushant @shaheemazmalmmd is the PR alright or does it need any more updates? Should I rebase the branch from base once more ?
Thankss !!

changes

Signed-off-by: Ritankar Saha <[email protected]>

php space fixes

Signed-off-by: Ritankar Saha <[email protected]>

fix(phpfatalerror): fixes uncaught error

Signed-off-by: Ritankar Saha <[email protected]>

fix(phpfatalerror): fixed uncaught error

Signed-off-by: Ritankar Saha <[email protected]>

feat(phpfatalerror): fixed indentation errors

Signed-off-by: Ritankar Saha <[email protected]>

fix(phpfatalerror): fixed uncaught error

Signed-off-by: Ritankar Saha <[email protected]>

fix(phpfatalerror): fixed uncaught error to member function

Signed-off-by: Ritankar Saha <[email protected]>
@shaheemazmalmmd shaheemazmalmmd force-pushed the ritankarsaha/fix/2833/phpfatalerror branch from d772683 to ffa55f7 Compare May 29, 2025 08:38
@shaheemazmalmmd shaheemazmalmmd merged commit 9bdb073 into fossology:master May 29, 2025
14 checks passed
@GMishx GMishx mentioned this pull request May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants